diff --git a/packages/config-loader/src/lib/schema/compile.test.ts b/packages/config-loader/src/lib/schema/compile.test.ts index e4ffc4801a..1e63f3cbf8 100644 --- a/packages/config-loader/src/lib/schema/compile.test.ts +++ b/packages/config-loader/src/lib/schema/compile.test.ts @@ -29,12 +29,28 @@ describe('compileConfigSchemas', () => { }, ]); expect(validate([{ data: { a: 1 }, context: 'test' }])).toEqual({ - errors: ['Config should be string { type=string } at /a'], + errors: [ + { + keyword: 'type', + dataPath: '/a', + schemaPath: '#/properties/a/type', + message: 'should be string', + params: { type: 'string' }, + }, + ], visibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), }); expect(validate([{ data: { b: 'b' }, context: 'test' }])).toEqual({ - errors: ['Config should be number { type=number } at /b'], + errors: [ + { + keyword: 'type', + dataPath: '/b', + schemaPath: '#/properties/b/type', + message: 'should be number', + params: { type: 'number' }, + }, + ], visibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), }); diff --git a/packages/config-loader/src/lib/schema/compile.ts b/packages/config-loader/src/lib/schema/compile.ts index 746a3a7a25..1cbf5a404d 100644 --- a/packages/config-loader/src/lib/schema/compile.ts +++ b/packages/config-loader/src/lib/schema/compile.ts @@ -95,16 +95,10 @@ export function compileConfigSchemas( const valid = validate(config); if (!valid) { - const errors = validate.errors ?? []; return { - errors: errors.map(({ dataPath, message, params }) => { - const paramStr = Object.entries(params) - .map(([name, value]) => `${name}=${value}`) - .join(' '); - return `Config ${message || ''} { ${paramStr} } at ${dataPath}`; - }), - visibilityByDataPath: new Map(), - visibilityBySchemaPath: new Map(), + errors: validate.errors ?? [], + visibilityByDataPath: new Map(visibilityByDataPath), + visibilityBySchemaPath, }; } diff --git a/packages/config-loader/src/lib/schema/filtering.ts b/packages/config-loader/src/lib/schema/filtering.ts index dc7fac0293..7cb122ee90 100644 --- a/packages/config-loader/src/lib/schema/filtering.ts +++ b/packages/config-loader/src/lib/schema/filtering.ts @@ -19,6 +19,7 @@ import { ConfigVisibility, DEFAULT_CONFIG_VISIBILITY, TransformFunc, + ValidationError, } from './types'; /** @@ -105,3 +106,54 @@ export function filterByVisibility( data: (transform(data, '', '') as JsonObject) ?? {}, }; } + +export function filterErrorsByVisibility( + errors: ValidationError[] | undefined, + includeVisibilities: ConfigVisibility[] | undefined, + visibilityByDataPath: Map, + visibilityBySchemaPath: Map, +): ValidationError[] { + if (!errors) { + return []; + } + if (!includeVisibilities) { + return errors; + } + + const visibleSchemaPaths = Array.from(visibilityBySchemaPath) + .filter(([, v]) => includeVisibilities.includes(v)) + .map(([k]) => k); + + // If we're filtering by visibility we only care about the errors that happened + // in a visible path. + return errors.filter(error => { + // We always include structural errors as we don't know whether there are + // any visible paths within the structures. + if ( + error.keyword === 'type' && + ['object', 'array'].includes(error.params.type) + ) { + return true; + } + + // For fields that were required we use the schema path to determine whether + // it was visible in addition to the data path. This is because the data path + // visibilities are only populated for values that we reached, which we won't + // if the value is missing. + // We don't use this method for all the errors as the data path is more robust + // and doesn't require us to properly trim the schema path. + if (error.keyword === 'required') { + const trimmedPath = error.schemaPath.slice(1, -'/required'.length); + if ( + visibleSchemaPaths.some(visiblePath => + visiblePath.startsWith(trimmedPath), + ) + ) { + return true; + } + } + + const vis = visibilityByDataPath.get(error.dataPath); + return vis && includeVisibilities.includes(vis); + }); +} diff --git a/packages/config-loader/src/lib/schema/load.test.ts b/packages/config-loader/src/lib/schema/load.test.ts index 73fa55b90c..e2ae3cc2c7 100644 --- a/packages/config-loader/src/lib/schema/load.test.ts +++ b/packages/config-loader/src/lib/schema/load.test.ts @@ -102,4 +102,146 @@ describe('loadConfigSchema', () => { 'Serialized configuration schema is invalid or has an invalid version number', ); }); + + describe('should consider schema', () => { + it('when filtering simple config', async () => { + mockFs({ + 'package.json': JSON.stringify({ + name: 'a', + configSchema: { + type: 'object', + properties: { + key1: { type: 'string', visibility: 'frontend' }, + key2: { type: 'number', visibility: 'secret' }, + }, + }, + }), + }); + + const schema = await loadConfigSchema({ + packagePaths: ['package.json'], + dependencies: [], + }); + + const configs = [ + { data: { key1: 'a', key2: 'not-a-number' }, context: 'test' }, + ]; + + expect(() => schema.process(configs)).toThrow( + 'Config validation failed, Config should be number { type=number } at /key2', + ); + expect(schema.process(configs, { visibility: ['frontend'] })).toEqual([ + { data: { key1: 'a' }, context: 'test' }, + ]); + expect(() => schema.process(configs, { visibility: ['secret'] })).toThrow( + 'Config validation failed, Config should be number { type=number } at /key2', + ); + }); + + it('when filtering nested config', async () => { + mockFs({ + 'package.json': JSON.stringify({ + name: 'a', + configSchema: { + type: 'object', + properties: { + nested: { + allOf: [ + { + type: 'array', + items: { + type: 'object', + visibility: 'frontend', + properties: { + x: { type: 'number' }, + }, + additionalProperties: { + visibility: 'frontend', + type: 'string', + pattern: '^...$', + }, + }, + }, + ], + }, + }, + }, + }), + }); + + const schema = await loadConfigSchema({ + packagePaths: ['package.json'], + dependencies: [], + }); + + const mkConfig = (nested: any) => [ + { data: { nested: [nested] }, context: 'test' }, + ]; + expect( + schema.process(mkConfig({ x: 1 }), { visibility: ['frontend'] }), + ).toEqual([{ data: { nested: [{}] }, context: 'test' }]); + expect(() => schema.process(mkConfig({ y: 1 }))).toThrow( + 'Config validation failed, Config should be string { type=string } at /nested/0/y', + ); + expect(() => + schema.process(mkConfig({ y: 1 }), { visibility: ['frontend'] }), + ).toThrow( + 'Config validation failed, Config should be string { type=string } at /nested/0/y', + ); + expect( + schema.process(mkConfig({ x: 'a' }), { visibility: ['frontend'] }), + ).toEqual([{ data: { nested: [{}] }, context: 'test' }]); + expect( + schema.process(mkConfig({ y: 'aaa' }), { visibility: ['frontend'] }), + ).toEqual([{ data: { nested: [{ y: 'aaa' }] }, context: 'test' }]); + expect(() => + schema.process(mkConfig({ y: 'aaaa' }), { visibility: ['frontend'] }), + ).toThrow( + 'Config validation failed, Config should match pattern "^...$" { pattern=^...$ } at /nested/0/y', + ); + + // This is a bit of an edge case where we have a structural error, these should always be reported + expect(() => + schema.process([{ data: { nested: {} }, context: 'test' }], { + visibility: ['frontend'], + }), + ).toThrow( + 'Config validation failed, Config should be array { type=array } at /nested', + ); + }); + }); + + it('when filtering config with required values', async () => { + mockFs({ + 'package.json': JSON.stringify({ + name: 'a', + configSchema: { + type: 'object', + properties: { + other: { + required: ['x a'], + type: 'object', + properties: { + 'x a': { type: 'number', visibility: 'frontend' }, + }, + }, + }, + }, + }), + }); + + const schema = await loadConfigSchema({ + packagePaths: ['package.json'], + dependencies: [], + }); + + // Errors about required values should also be filtered like the rest + expect(() => + schema.process([{ data: { other: {} }, context: 'test' }], { + visibility: ['frontend'], + }), + ).toThrow( + "Config should have required property 'x a' { missingProperty=x a } at /other", + ); + }); }); diff --git a/packages/config-loader/src/lib/schema/load.ts b/packages/config-loader/src/lib/schema/load.ts index 60402d5237..0e9ddabcb3 100644 --- a/packages/config-loader/src/lib/schema/load.ts +++ b/packages/config-loader/src/lib/schema/load.ts @@ -17,8 +17,9 @@ import { AppConfig, JsonObject } from '@backstage/config'; import { compileConfigSchemas } from './compile'; import { collectConfigSchemas } from './collect'; -import { filterByVisibility } from './filtering'; +import { filterByVisibility, filterErrorsByVisibility } from './filtering'; import { + ValidationError, ConfigSchema, ConfigSchemaPackageEntry, CONFIG_VISIBILITIES, @@ -34,6 +35,18 @@ export type LoadConfigSchemaOptions = serialized: JsonObject; }; +function errorsToError(errors: ValidationError[]): Error { + const messages = errors.map(({ dataPath, message, params }) => { + const paramStr = Object.entries(params) + .map(([name, value]) => `${name}=${value}`) + .join(' '); + return `Config ${message || ''} { ${paramStr} } at ${dataPath}`; + }); + const error = new Error(`Config validation failed, ${messages.join('; ')}`); + (error as any).messages = messages; + return error; +} + /** * Loads config schema for a Backstage instance. * @@ -67,12 +80,15 @@ export async function loadConfigSchema( { visibility, valueTransform, withFilteredKeys } = {}, ): AppConfig[] { const result = validate(configs); - if (result.errors) { - const error = new Error( - `Config validation failed, ${result.errors.join('; ')}`, - ); - (error as any).messages = result.errors; - throw error; + + const visibleErrors = filterErrorsByVisibility( + result.errors, + visibility, + result.visibilityByDataPath, + result.visibilityBySchemaPath, + ); + if (visibleErrors.length > 0) { + throw errorsToError(visibleErrors); } let processedConfigs = configs; diff --git a/packages/config-loader/src/lib/schema/types.ts b/packages/config-loader/src/lib/schema/types.ts index 6763b3e1e1..d1c07f4dac 100644 --- a/packages/config-loader/src/lib/schema/types.ts +++ b/packages/config-loader/src/lib/schema/types.ts @@ -50,7 +50,14 @@ export const DEFAULT_CONFIG_VISIBILITY: ConfigVisibility = 'backend'; /** * An explanation of a configuration validation error. */ -type ValidationError = string; +export type ValidationError = { + keyword: string; + dataPath: string; + schemaPath: string; + params: Record; + propertyName?: string; + message?: string; +}; /** * The result of validating configuration data using a schema.