From 9606ba0939e6b8be0182ae9a1f58c0494518e2a2 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 21 Aug 2023 17:44:16 +0200 Subject: [PATCH] config-loader: apply deep visibility to all values Signed-off-by: Patrik Oldsberg --- .changeset/sweet-pens-hunt.md | 24 +++++++ .../config-loader/src/schema/compile.test.ts | 59 +++++++++------- packages/config-loader/src/schema/compile.ts | 67 ++++++++++++------- .../src/schema/filtering.test.ts | 44 +++++++++++- .../config-loader/src/schema/filtering.ts | 20 +++++- packages/config-loader/src/schema/load.ts | 2 + packages/config-loader/src/schema/types.ts | 7 ++ 7 files changed, 167 insertions(+), 56 deletions(-) create mode 100644 .changeset/sweet-pens-hunt.md diff --git a/.changeset/sweet-pens-hunt.md b/.changeset/sweet-pens-hunt.md new file mode 100644 index 0000000000..03e79ecebb --- /dev/null +++ b/.changeset/sweet-pens-hunt.md @@ -0,0 +1,24 @@ +--- +'@backstage/config-loader': minor +--- + +Deep visibility now also applies to values that are not covered by the configuration schema. + +For example, given the following configuration schema: + +```ts +// plugins/a/config.schema.ts +export interface Config { + /** @deepVisibility frontend */ + a?: unknown; +} + +// plugins/a/config.schema.ts +export interface Config { + a?: { + b?: string; + }; +} +``` + +All values under `a` are now visible to the frontend, while previously only `a` and `a/b` would've been visible. diff --git a/packages/config-loader/src/schema/compile.test.ts b/packages/config-loader/src/schema/compile.test.ts index 914a813d9f..ff0a9543aa 100644 --- a/packages/config-loader/src/schema/compile.test.ts +++ b/packages/config-loader/src/schema/compile.test.ts @@ -39,6 +39,7 @@ describe('compileConfigSchemas', () => { }, ], visibilityByDataPath: new Map(), + deepVisibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), deprecationByDataPath: new Map(), }); @@ -53,6 +54,7 @@ describe('compileConfigSchemas', () => { }, ], visibilityByDataPath: new Map(), + deepVisibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), deprecationByDataPath: new Map(), }); @@ -106,6 +108,7 @@ describe('compileConfigSchemas', () => { '/d/0': 'frontend', }), ), + deepVisibilityByDataPath: new Map(), visibilityBySchemaPath: new Map( Object.entries({ '/properties/a': 'frontend', @@ -166,6 +169,7 @@ describe('compileConfigSchemas', () => { '/b': 'deprecation reason for b', }), ), + deepVisibilityByDataPath: new Map(), visibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), }); @@ -221,6 +225,7 @@ describe('compileConfigSchemas', () => { '//circleci/api/headers/Circle-Token': 'secret', }), ), + deepVisibilityByDataPath: new Map(), visibilityBySchemaPath: new Map( Object.entries({ '/properties//circleci/api/properties/headers/properties/Circle-Token': @@ -277,18 +282,18 @@ describe('deepVisibility', () => { ).toEqual({ visibilityByDataPath: new Map( Object.entries({ - '': 'secret', '/b': 'secret', - '/d': 'secret', - '/d/0': 'secret', + }), + ), + deepVisibilityByDataPath: new Map( + Object.entries({ + '': 'secret', }), ), visibilityBySchemaPath: new Map( Object.entries({ '': 'secret', '/properties/b': 'secret', - '/properties/d': 'secret', - '/properties/d/items': 'secret', }), ), deprecationByDataPath: new Map(), @@ -339,26 +344,6 @@ describe('deepVisibility', () => { it('should throw when children have a different deepVisibility', () => { expect(() => compileConfigSchemas([ - { - path: 'a1', - value: { - type: 'object', - properties: { - a: { - type: 'object', - properties: { - a: { type: 'string' }, - }, - }, - b: { type: 'string', visibility: 'backend' }, - c: { type: 'string' }, - d: { - type: 'array', - items: { type: 'string' }, - }, - }, - }, - }, { path: 'a2', value: { @@ -368,7 +353,7 @@ describe('deepVisibility', () => { a: { type: 'object', properties: { - a: { type: 'string', visibility: 'frontend' }, + a: { type: 'string', deepVisibility: 'frontend' }, }, }, b: { type: 'string', visibility: 'secret' }, @@ -386,6 +371,28 @@ describe('deepVisibility', () => { ); }); + it('should throw when the same schema node has a conflicting deepVisibility', () => { + expect(() => + compileConfigSchemas([ + { + path: 'a2', + value: { + type: 'object', + properties: { + a: { + type: 'string', + deepVisibility: 'secret', + visibility: 'frontend', + }, + }, + }, + }, + ]), + ).toThrow( + `Config schema visibility is both 'frontend' and 'secret' for /properties/a`, + ); + }); + it('should throw when ancestor and children have a different deepVisibility', () => { expect(() => compileConfigSchemas([ diff --git a/packages/config-loader/src/schema/compile.ts b/packages/config-loader/src/schema/compile.ts index 5d448a590a..fd39b0def9 100644 --- a/packages/config-loader/src/schema/compile.ts +++ b/packages/config-loader/src/schema/compile.ts @@ -28,6 +28,9 @@ import { import { SchemaObject } from 'json-schema-traverse'; import { normalizeAjvPath } from './utils'; +// Used to keep track of the internal deepVisibility inherited through the schema. +const inheritedVisibility = Symbol('inherited-visibility'); + /** * This takes a collection of Backstage configuration schemas from various * sources and compiles them down into a single schema validation function. @@ -45,6 +48,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 deepVisibilityByDataPath = new Map(); const deprecationByDataPath = new Map(); const ajv = new Ajv({ @@ -87,6 +91,18 @@ export function compileConfigSchemas( */ enum: ['frontend', 'secret'], }, + compile(visibility: 'frontend' | 'secret') { + return (_data, context) => { + if (context?.instancePath === undefined) { + return false; + } + if (visibility) { + const normalizedPath = normalizeAjvPath(context.instancePath); + deepVisibilityByDataPath.set(normalizedPath, visibility); + } + return true; + }; + }, }) .removeKeyword('deprecated') // remove `deprecated` keyword so that we can implement our own compiler .addKeyword({ @@ -118,37 +134,34 @@ export function compileConfigSchemas( traverse( merged, ( - schema: SchemaObject, + schema: SchemaObject & { [inheritedVisibility]?: ConfigVisibility }, jsonPtr: string, _1: any, _2: any, _3?: any, - parentSchema?: SchemaObject, + parentSchema?: SchemaObject & { + [inheritedVisibility]?: ConfigVisibility; + }, ) => { // Inherit parent deepVisibility if we don't define one ourselves. - if (parentSchema?.deepVisibility) { - schema.deepVisibility ??= parentSchema?.deepVisibility; - } + // This is used to detect situations where conflicting deep visibilities are set. + schema[inheritedVisibility] ??= + schema?.deepVisibility ?? parentSchema?.[inheritedVisibility]; - // Apply deep visibility to self. - if (schema?.deepVisibility) { - // This runs before we compile the AJV so visibilityByDataPath has the - // correct data. - schema.visibility ??= schema.deepVisibility; - - if (parentSchema) { - /** - * Validate that we're not trying to override a child's visibility - * by setting the parent deep visibility. - */ - const values = [schema.visibility, parentSchema.visibility]; - const hasFrontend = values.some(e => e === 'frontend'); - const hasSecret = values.some(e => e === 'secret'); - if (hasFrontend && hasSecret) { - throw new Error( - `Config schema visibility is both 'frontend' and 'secret' for ${jsonPtr}`, - ); - } + if (schema[inheritedVisibility]) { + // Validate that we're not trying to set a conflicting visibility. This can be done + // either by setting a conflicting visibility directly or deep visibility + const values = [ + schema.visibility, + schema[inheritedVisibility], + parentSchema?.[inheritedVisibility], + ]; + const hasFrontend = values.some(e => e === 'frontend'); + const hasSecret = values.some(e => e === 'secret'); + if (hasFrontend && hasSecret) { + throw new Error( + `Config schema visibility is both 'frontend' and 'secret' for ${jsonPtr}`, + ); } } @@ -171,12 +184,16 @@ export function compileConfigSchemas( if (schema.visibility && schema.visibility !== 'backend') { visibilityBySchemaPath.set(normalizeAjvPath(path), schema.visibility); } + if (schema.deepVisibility) { + visibilityBySchemaPath.set(normalizeAjvPath(path), schema.deepVisibility); + } }); return configs => { const config = ConfigReader.fromConfigs(configs).get(); visibilityByDataPath.clear(); + deepVisibilityByDataPath.clear(); const valid = validate(config); @@ -184,6 +201,7 @@ export function compileConfigSchemas( return { errors: validate.errors ?? [], visibilityByDataPath: new Map(visibilityByDataPath), + deepVisibilityByDataPath: new Map(deepVisibilityByDataPath), visibilityBySchemaPath, deprecationByDataPath, }; @@ -191,6 +209,7 @@ export function compileConfigSchemas( return { visibilityByDataPath: new Map(visibilityByDataPath), + deepVisibilityByDataPath: new Map(deepVisibilityByDataPath), visibilityBySchemaPath, deprecationByDataPath, }; diff --git a/packages/config-loader/src/schema/filtering.test.ts b/packages/config-loader/src/schema/filtering.test.ts index 3a5de7e2c0..4beed367ea 100644 --- a/packages/config-loader/src/schema/filtering.test.ts +++ b/packages/config-loader/src/schema/filtering.test.ts @@ -30,6 +30,8 @@ const data = { b: { s: true, }, + df: { a: 1, b: [{ c: 1 }, '1'], d: { e: 'x' } }, + ds: { a: 1, b: [{ c: 1 }, '1'], d: { e: 'x' } }, }, arrF: [{ never: 'here' }], arrB: [{ never: 'here' }], @@ -64,6 +66,13 @@ const visibility = new Map( }), ); +const deepVisibility = new Map( + Object.entries({ + '/obj/df': 'frontend', + '/obj/ds': 'secret', + }), +); + const deprecations = new Map( Object.entries({ '/arr': 'deprecated array', @@ -92,6 +101,14 @@ describe('filterByVisibility', () => { 'objArr[1].s', 'obj.f', 'obj.b.s', + 'obj.df.a', + 'obj.df.b[0].c', + 'obj.df.b[1]', + 'obj.df.d.e', + 'obj.ds.a', + 'obj.ds.b[0].c', + 'obj.ds.b[1]', + 'obj.ds.d.e', 'arrF[0].never', 'arrB[0].never', 'arrS[0].never', @@ -107,7 +124,7 @@ describe('filterByVisibility', () => { data: { arr: ['f'], objArr: [{ f: 1 }, { f: 4 }], - obj: { f: 'a' }, + obj: { f: 'a', df: { a: 1, b: [{ c: 1 }, '1'], d: { e: 'x' } } }, arrF: [], objF: {}, arrU: ['f', 'b'], @@ -121,6 +138,10 @@ describe('filterByVisibility', () => { 'objArr[1].b', 'objArr[1].s', 'obj.b.s', + 'obj.ds.a', + 'obj.ds.b[0].c', + 'obj.ds.b[1]', + 'obj.ds.d.e', 'arrF[0].never', 'arrB[0].never', 'arrS[0].never', @@ -137,7 +158,7 @@ describe('filterByVisibility', () => { arr: ['b'], arrU: ['t'], objArr: [{ b: 2 }, { b: 5 }], - obj: { b: {} }, + obj: { b: {}, df: {}, ds: {} }, arrF: [{ never: 'here' }], arrB: [{ never: 'here' }], arrS: [{ never: 'here' }], @@ -156,6 +177,14 @@ describe('filterByVisibility', () => { 'objArr[1].s', 'obj.f', 'obj.b.s', + 'obj.df.a', + 'obj.df.b[0].c', + 'obj.df.b[1]', + 'obj.df.d.e', + 'obj.ds.a', + 'obj.ds.b[0].c', + 'obj.ds.b[1]', + 'obj.ds.d.e', ], }, ], @@ -165,7 +194,10 @@ describe('filterByVisibility', () => { data: { arr: ['s'], objArr: [{ s: 3 }, { s: 6 }], - obj: { b: { s: true } }, + obj: { + b: { s: true }, + ds: { a: 1, b: [{ c: 1 }, '1'], d: { e: 'x' } }, + }, arrS: [], objS: {}, }, @@ -180,6 +212,10 @@ describe('filterByVisibility', () => { 'objArr[1].f', 'objArr[1].b', 'obj.f', + 'obj.df.a', + 'obj.df.b[0].c', + 'obj.df.b[1]', + 'obj.df.d.e', 'arrF[0].never', 'arrB[0].never', 'arrS[0].never', @@ -196,6 +232,7 @@ describe('filterByVisibility', () => { data, filter, visibility, + deepVisibility, deprecations, undefined, true, @@ -210,6 +247,7 @@ describe('filterByVisibility', () => { data, [], visibility, + deepVisibility, deprecations, undefined, true, diff --git a/packages/config-loader/src/schema/filtering.ts b/packages/config-loader/src/schema/filtering.ts index 1c2d0c122a..1b7bbf43c1 100644 --- a/packages/config-loader/src/schema/filtering.ts +++ b/packages/config-loader/src/schema/filtering.ts @@ -31,6 +31,7 @@ export function filterByVisibility( data: JsonObject, includeVisibilities: ConfigVisibility[], visibilityByDataPath: Map, + deepVisibilityByDataPath: Map, deprecationByDataPath: Map, transformFunc?: TransformFunc, withFilteredKeys?: boolean, @@ -47,11 +48,17 @@ export function filterByVisibility( jsonVal: JsonValue, visibilityPath: string, // Matches the format we get from ajv filterPath: string, // Matches the format of the ConfigReader + inheritedVisibility: ConfigVisibility, ): JsonValue | undefined { const visibility = - visibilityByDataPath.get(visibilityPath) ?? DEFAULT_CONFIG_VISIBILITY; + visibilityByDataPath.get(visibilityPath) ?? inheritedVisibility; const isVisible = includeVisibilities.includes(visibility); + // If a deep visibility is set for our current path, then we that as our + // default visibility for all children until we encounter a different deep visibility + const newInheritedVisibility = + deepVisibilityByDataPath.get(visibilityPath) ?? inheritedVisibility; + // deprecated keys are added regardless of visibility indicator const deprecation = deprecationByDataPath.get(visibilityPath); if (deprecation) { @@ -84,7 +91,12 @@ export function filterByVisibility( path = `${visibilityPath}/${index}`; } - const out = transform(value, path, `${filterPath}[${index}]`); + const out = transform( + value, + path, + `${filterPath}[${index}]`, + newInheritedVisibility, + ); if (out !== undefined) { arr.push(out); @@ -108,6 +120,7 @@ export function filterByVisibility( value, `${visibilityPath}/${key}`, filterPath ? `${filterPath}.${key}` : key, + newInheritedVisibility, ); if (out !== undefined) { outObj[key] = out; @@ -124,7 +137,8 @@ export function filterByVisibility( return { filteredKeys: withFilteredKeys ? filteredKeys : undefined, deprecatedKeys: withDeprecatedKeys ? deprecatedKeys : undefined, - data: (transform(data, '', '') as JsonObject) ?? {}, + data: + (transform(data, '', '', DEFAULT_CONFIG_VISIBILITY) as JsonObject) ?? {}, }; } diff --git a/packages/config-loader/src/schema/load.ts b/packages/config-loader/src/schema/load.ts index 8ef971184f..885da44116 100644 --- a/packages/config-loader/src/schema/load.ts +++ b/packages/config-loader/src/schema/load.ts @@ -122,6 +122,7 @@ export async function loadConfigSchema( data, visibility, result.visibilityByDataPath, + result.deepVisibilityByDataPath, result.deprecationByDataPath, valueTransform, withFilteredKeys, @@ -135,6 +136,7 @@ export async function loadConfigSchema( data, Array.from(CONFIG_VISIBILITIES), result.visibilityByDataPath, + result.deepVisibilityByDataPath, result.deprecationByDataPath, valueTransform, withFilteredKeys, diff --git a/packages/config-loader/src/schema/types.ts b/packages/config-loader/src/schema/types.ts index 95576cf5aa..dc8da64038 100644 --- a/packages/config-loader/src/schema/types.ts +++ b/packages/config-loader/src/schema/types.ts @@ -75,6 +75,13 @@ type ValidationResult = { */ visibilityByDataPath: Map; + /** + * The configuration deep visibilities that were discovered during validation. + * + * The path in the key uses the form `////` + */ + deepVisibilityByDataPath: Map; + /** * The configuration visibilities that were discovered during validation. *