diff --git a/packages/config-loader/src/lib/schema/collect.ts b/packages/config-loader/src/lib/schema/collect.ts index aa3202a1b5..a05796139b 100644 --- a/packages/config-loader/src/lib/schema/collect.ts +++ b/packages/config-loader/src/lib/schema/collect.ts @@ -182,7 +182,7 @@ function compileTsSchemas(paths: string[]) { program, // All schemas should export a `Config` symbol 'Config', - // This enables usage of @visibility and @deprecated is doc comments + // This enables usage of @visibility and @deprecated in doc comments { required: true, validationKeywords: ['visibility', 'deprecated'], diff --git a/packages/config-loader/src/lib/schema/compile.test.ts b/packages/config-loader/src/lib/schema/compile.test.ts index 7615fa80f5..7b7db87c8f 100644 --- a/packages/config-loader/src/lib/schema/compile.test.ts +++ b/packages/config-loader/src/lib/schema/compile.test.ts @@ -40,7 +40,7 @@ describe('compileConfigSchemas', () => { ], visibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), - deprecationBySchemaPath: new Map(), + deprecationByDataPath: new Map(), }); expect(validate([{ data: { b: 'b' }, context: 'test' }])).toEqual({ errors: [ @@ -54,7 +54,7 @@ describe('compileConfigSchemas', () => { ], visibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), - deprecationBySchemaPath: new Map(), + deprecationByDataPath: new Map(), }); }); @@ -114,7 +114,7 @@ describe('compileConfigSchemas', () => { '/properties/d/items': 'frontend', }), ), - deprecationBySchemaPath: new Map(), + deprecationByDataPath: new Map(), }); }); @@ -160,10 +160,10 @@ describe('compileConfigSchemas', () => { { data: { a: 'a', b: 'b', c: 'c', d: ['d'] }, context: 'test' }, ]), ).toEqual({ - deprecationBySchemaPath: new Map( + deprecationByDataPath: new Map( Object.entries({ - '/properties/a': 'deprecation reason for a', - '/properties/b': 'deprecation reason for b', + '/a': 'deprecation reason for a', + '/b': 'deprecation reason for b', }), ), visibilityByDataPath: new Map(), diff --git a/packages/config-loader/src/lib/schema/compile.ts b/packages/config-loader/src/lib/schema/compile.ts index 6dcd4713f5..b7f705751e 100644 --- a/packages/config-loader/src/lib/schema/compile.ts +++ b/packages/config-loader/src/lib/schema/compile.ts @@ -40,6 +40,7 @@ export function compileConfigSchemas( // output during validation. We work around this by having this extra piece // of state that we reset before each validation. const visibilityByDataPath = new Map(); + const deprecationByDataPath = new Map(); const ajv = new Ajv({ allErrors: true, @@ -47,28 +48,48 @@ export function compileConfigSchemas( schemas: { 'https://backstage.io/schema/config-v1': true, }, - }).addKeyword({ - keyword: 'visibility', - metaSchema: { - type: 'string', - enum: CONFIG_VISIBILITIES, - }, - compile(visibility: ConfigVisibility) { - return (_data, context) => { - if (context?.dataPath === undefined) { - return false; - } - if (visibility && visibility !== 'backend') { + }) + .addKeyword({ + keyword: 'visibility', + metaSchema: { + type: 'string', + enum: CONFIG_VISIBILITIES, + }, + compile(visibility: ConfigVisibility) { + return (_data, context) => { + if (context?.dataPath === undefined) { + return false; + } + if (visibility && visibility !== 'backend') { + const normalizedPath = context.dataPath.replace( + /\['?(.*?)'?\]/g, + (_, segment) => `/${segment}`, + ); + visibilityByDataPath.set(normalizedPath, visibility); + } + return true; + }; + }, + }) + .removeKeyword('deprecated') // remove `deprecated` keyword so that we can implement our own compiler + .addKeyword({ + keyword: 'deprecated', + metaSchema: { type: 'string' }, + compile(deprecationDescription: string) { + return (_data, context) => { + if (context?.dataPath === undefined) { + return false; + } const normalizedPath = context.dataPath.replace( /\['?(.*?)'?\]/g, (_, segment) => `/${segment}`, ); - visibilityByDataPath.set(normalizedPath, visibility); - } - return true; - }; - }, - }); + // create mapping of deprecation description and data path of property + deprecationByDataPath.set(normalizedPath, deprecationDescription); + return true; + }; + }, + }); for (const schema of schemas) { try { @@ -79,14 +100,8 @@ export function compileConfigSchemas( } const merged = mergeConfigSchemas(schemas.map(_ => _.value)); - const validate = ajv.compile(merged); - const deprecationBySchemaPath = new Map(); - traverse(merged, (schema, path) => { - if ('deprecated' in schema) { - deprecationBySchemaPath.set(path, schema.deprecated); - } - }); + const validate = ajv.compile(merged); const visibilityBySchemaPath = new Map(); traverse(merged, (schema, path) => { @@ -101,19 +116,20 @@ export function compileConfigSchemas( visibilityByDataPath.clear(); const valid = validate(config); + if (!valid) { return { errors: validate.errors ?? [], visibilityByDataPath: new Map(visibilityByDataPath), visibilityBySchemaPath, - deprecationBySchemaPath, + deprecationByDataPath, }; } return { visibilityByDataPath: new Map(visibilityByDataPath), visibilityBySchemaPath, - deprecationBySchemaPath, + deprecationByDataPath, }; }; } diff --git a/packages/config-loader/src/lib/schema/filtering.test.ts b/packages/config-loader/src/lib/schema/filtering.test.ts index 2324bebc9d..df06975c9f 100644 --- a/packages/config-loader/src/lib/schema/filtering.test.ts +++ b/packages/config-loader/src/lib/schema/filtering.test.ts @@ -16,11 +16,7 @@ import { JsonObject } from '@backstage/types'; import { ConfigVisibility } from './types'; -import { - filterByVisibility, - filterErrorsByVisibility, - filterByDeprecated, -} from './filtering'; +import { filterByVisibility, filterErrorsByVisibility } from './filtering'; const data = { arr: ['f', 'b', 's'], @@ -70,8 +66,8 @@ const visibility = new Map( const deprecations = new Map( Object.entries({ - '/properties/arr': 'deprecated array', - '/properties/objB/properties/never': 'deprecated nested property', + '/arr': 'deprecated array', + '/objB/never': 'deprecated nested property', }), ); @@ -196,9 +192,34 @@ describe('filterByVisibility', () => { [['frontend', 'backend', 'secret'], { data, filteredKeys: [] }], ])('should filter correctly with %p', (filter, expected) => { expect( - filterByVisibility(data, filter, visibility, undefined, true), + filterByVisibility( + data, + filter, + visibility, + deprecations, + undefined, + true, + false, + ), ).toEqual(expected); }); + + it('should include deprecated keys regardless of visibility', () => { + expect( + filterByVisibility( + data, + [], + visibility, + deprecations, + undefined, + true, + true, + ).deprecatedKeys, + ).toEqual([ + { key: 'arr', description: 'deprecated array' }, + { key: 'objB.never', description: 'deprecated nested property' }, + ]); + }); }); describe('filterErrorsByVisibility', () => { @@ -392,17 +413,3 @@ describe('filterErrorsByVisibility', () => { ]); }); }); - -describe('filterByDeprecated', () => { - it('should allow empty list of deprecations', () => { - expect(filterByDeprecated(data, new Map())).toEqual({ deprecatedKeys: [] }); - }); - it('should return a list of deprecated properties', () => { - expect(filterByDeprecated(data, deprecations)).toEqual({ - deprecatedKeys: [ - { key: 'arr', description: 'deprecated array' }, - { key: 'objB.never', description: 'deprecated nested property' }, - ], - }); - }); -}); diff --git a/packages/config-loader/src/lib/schema/filtering.ts b/packages/config-loader/src/lib/schema/filtering.ts index f955343066..11ee4cb47f 100644 --- a/packages/config-loader/src/lib/schema/filtering.ts +++ b/packages/config-loader/src/lib/schema/filtering.ts @@ -23,36 +23,6 @@ import { ValidationError, } from './types'; -/** - * This filters data by deprecation status to only include those which have been deprecated - * - * @param data - configuration data - * @param deprecationBySchemaPath - mapping of schema path to deprecation description - * @returns deprecated configuration keys with their deprecation description - */ -export function filterByDeprecated( - data: JsonObject, - deprecationBySchemaPath: Map, - withDeprecatedKeys: boolean = true, -): { deprecatedKeys: { key: string; description: string }[] } { - const deprecatedKeys = new Array<{ key: string; description: string }>(); - if (!withDeprecatedKeys) { - return { deprecatedKeys }; - } - - deprecationBySchemaPath.forEach((desc, schemaPath) => { - // convert schema path to object path (/properties/techdocs/properties/storageUrl -> techdocs.storageUrl) - const propertyPath = schemaPath - .replace(/^\/properties\//, '') // remove leading `/properties/` entirely - .replace(/\/properties\//g, '.'); // replace all other `/properties/` with a `.` - if (has(data, propertyPath)) { - deprecatedKeys.push({ key: propertyPath, description: desc }); - } - }); - - return { deprecatedKeys }; -} - /** * This filters data by visibility by discovering the visibility of each * value, and then only keeping the ones that are specified in `includeVisibilities`. @@ -61,10 +31,17 @@ export function filterByVisibility( data: JsonObject, includeVisibilities: ConfigVisibility[], visibilityByDataPath: Map, + deprecationByDataPath: Map, transformFunc?: TransformFunc, withFilteredKeys?: boolean, -): { data: JsonObject; filteredKeys?: string[] } { + withDeprecatedKeys?: boolean, +): { + data: JsonObject; + filteredKeys?: string[]; + deprecatedKeys?: { key: string; description: string }[]; +} { const filteredKeys = new Array(); + const deprecatedKeys = new Array<{ key: string; description: string }>(); function transform( jsonVal: JsonValue, @@ -75,6 +52,12 @@ export function filterByVisibility( visibilityByDataPath.get(visibilityPath) ?? DEFAULT_CONFIG_VISIBILITY; const isVisible = includeVisibilities.includes(visibility); + // deprecated keys are added regardless of visibility indicator + const deprecation = deprecationByDataPath.get(visibilityPath); + if (deprecation) { + deprecatedKeys.push({ key: filterPath, description: deprecation }); + } + if (typeof jsonVal !== 'object') { if (isVisible) { if (transformFunc) { @@ -140,6 +123,7 @@ export function filterByVisibility( return { filteredKeys: withFilteredKeys ? filteredKeys : undefined, + deprecatedKeys: withDeprecatedKeys ? deprecatedKeys : 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 a2f1df8d5f..8510ccb1d8 100644 --- a/packages/config-loader/src/lib/schema/load.test.ts +++ b/packages/config-loader/src/lib/schema/load.test.ts @@ -65,7 +65,6 @@ describe('loadConfigSchema', () => { { data: { key1: 'a' }, context: 'test', - deprecatedKeys: [], }, ]); expect( @@ -79,7 +78,6 @@ describe('loadConfigSchema', () => { data: { key1: 'X' }, context: 'test', filteredKeys: ['key2'], - deprecatedKeys: [], }, ]); expect( @@ -92,7 +90,6 @@ describe('loadConfigSchema', () => { data: { key1: 'X', key2: 'X' }, context: 'test', filteredKeys: [], - deprecatedKeys: [], }, ]); @@ -103,7 +100,6 @@ describe('loadConfigSchema', () => { { data: { key1: 'a' }, context: 'test', - deprecatedKeys: [], }, ]); expect(() => @@ -152,7 +148,6 @@ describe('loadConfigSchema', () => { { data: { key1: 'a' }, context: 'test', - deprecatedKeys: [], }, ]); expect(() => schema.process(configs, { visibility: ['secret'] })).toThrow( @@ -205,7 +200,6 @@ describe('loadConfigSchema', () => { { data: { nested: [{}] }, context: 'test', - deprecatedKeys: [], }, ]); expect(() => schema.process(mkConfig({ y: 1 }))).toThrow( @@ -222,7 +216,6 @@ describe('loadConfigSchema', () => { { data: { nested: [{}] }, context: 'test', - deprecatedKeys: [], }, ]); expect( @@ -231,7 +224,6 @@ describe('loadConfigSchema', () => { { data: { nested: [{ y: 'aaa' }] }, context: 'test', - deprecatedKeys: [], }, ]); expect(() => diff --git a/packages/config-loader/src/lib/schema/load.ts b/packages/config-loader/src/lib/schema/load.ts index 303fdfade9..9eac629ee3 100644 --- a/packages/config-loader/src/lib/schema/load.ts +++ b/packages/config-loader/src/lib/schema/load.ts @@ -109,12 +109,9 @@ export async function loadConfigSchema( data, visibility, result.visibilityByDataPath, + result.deprecationByDataPath, valueTransform, withFilteredKeys, - ), - ...filterByDeprecated( - data, - result.deprecationBySchemaPath, withDeprecatedKeys, ), })); @@ -125,12 +122,9 @@ export async function loadConfigSchema( data, Array.from(CONFIG_VISIBILITIES), result.visibilityByDataPath, + result.deprecationByDataPath, valueTransform, withFilteredKeys, - ), - ...filterByDeprecated( - data, - result.deprecationBySchemaPath, withDeprecatedKeys, ), })); diff --git a/packages/config-loader/src/lib/schema/types.ts b/packages/config-loader/src/lib/schema/types.ts index 76cc337f83..75993b3c78 100644 --- a/packages/config-loader/src/lib/schema/types.ts +++ b/packages/config-loader/src/lib/schema/types.ts @@ -85,9 +85,9 @@ type ValidationResult = { /** * The deprecated options that were discovered during validation. * - * The path in the key uses the form `/properties//items/additionalProperties/` + * The path in the key uses the form `////` */ - deprecationBySchemaPath: Map; + deprecationByDataPath: Map; }; /** diff --git a/packages/config/src/reader.ts b/packages/config/src/reader.ts index 5c34b215ab..d85ea6f482 100644 --- a/packages/config/src/reader.ts +++ b/packages/config/src/reader.ts @@ -87,20 +87,15 @@ export class ConfigReader implements Config { const reader = new ConfigReader(data, context, previousReader); reader.filteredKeys = filteredKeys; - // TODO(cmpadden) introduce `deprecatedKeys` and `notifiedDeprecatedKeys` & use logger + // TODO(cmpadden) `withDeprecatedKeys` is defaulting to false on app start if (deprecatedKeys) { for (const { key, description } of deprecatedKeys) { - if (description) { - // eslint-disable-next-line no-console - console.warn( - `The configuration key '${key}' is deprecated and may be removed soon. (reason: ${description})`, - ); - } else { - // eslint-disable-next-line no-console - console.warn( - `The configuration key '${key}' is deprecated and may be removed soon.`, - ); - } + // eslint-disable-next-line no-console + console.warn( + `The configuration key '${key}' of ${context} is deprecated and may be removed soon. ${ + description || '' + }`, + ); } }