paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 37 Packages Projects Releases Wiki Activity

Merge pull request #7634 from backstage/rugvip/filters

Browse Source 
config-loader: apply visibility filter to configuration schema validation errors
This commit is contained in:
Patrik Oldsberg
2021-10-15 15:39:06 +02:00
committed by GitHub
parent c5664a1307 223e8de6b4
commit 59e822b43c
9 changed files with 483 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.changeset/chilled-tools-worry.md
+5
View File
@@ -0,0 +1,5 @@
---
'@backstage/config-loader': patch
---
Configuration schema errors are now filtered using the provided visibility option. This means that schema errors due to missing backend configuration will no longer break frontend builds.
packages/config-loader/package.json
+1
View File
@@ -38,6 +38,7 @@
"fs-extra": "9.1.0",
"json-schema": "^0.3.0",
"json-schema-merge-allof": "^0.8.1",
"json-schema-traverse": "^1.0.0",
"typescript-json-schema": "^0.50.1",
"yaml": "^1.9.2",
"yup": "^0.32.9"
packages/config-loader/src/lib/schema/compile.test.ts
+31 -5
View File
@@ -29,12 +29,30 @@ describe('compileConfigSchemas', () => {
},
]);
expect(validate([{ data: { a: 1 }, context: 'test' }])).toEqual({
errors: ['Config should be string { type=string } at /a'],
visibilityByPath: new Map(),
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'],
visibilityByPath: new Map(),
errors: [
{
keyword: 'type',
dataPath: '/b',
schemaPath: '#/properties/b/type',
message: 'should be number',
params: { type: 'number' },
},
],
visibilityByDataPath: new Map(),
visibilityBySchemaPath: new Map(),
});
});
@@ -78,7 +96,7 @@ describe('compileConfigSchemas', () => {
{ data: { a: 'a', b: 'b', c: 'c', d: ['d'] }, context: 'test' },
]),
).toEqual({
visibilityByPath: new Map(
visibilityByDataPath: new Map(
Object.entries({
'/a': 'frontend',
'/b': 'secret',
@@ -86,6 +104,14 @@ describe('compileConfigSchemas', () => {
'/d/0': 'frontend',
}),
),
visibilityBySchemaPath: new Map(
Object.entries({
'/properties/a': 'frontend',
'/properties/b': 'secret',
'/properties/d': 'secret',
'/properties/d/items': 'frontend',
}),
),
});
});
packages/config-loader/src/lib/schema/compile.ts
+16 -12
View File
@@ -17,6 +17,7 @@
import Ajv from 'ajv';
import { JSONSchema7 as JSONSchema } from 'json-schema';
import mergeAllOf, { Resolvers } from 'json-schema-merge-allof';
import traverse from 'json-schema-traverse';
import { ConfigReader } from '@backstage/config';
import {
ConfigSchemaPackageEntry,
@@ -38,7 +39,7 @@ export function compileConfigSchemas(
// The ajv instance below is stateful and doesn't really allow for additional
// output during validation. We work around this by having this extra piece
// of state that we reset before each validation.
const visibilityByPath = new Map<string, ConfigVisibility>();
const visibilityByDataPath = new Map<string, ConfigVisibility>();
const ajv = new Ajv({
allErrors: true,
@@ -62,7 +63,7 @@ export function compileConfigSchemas(
/\['?(.*?)'?\]/g,
(_, segment) => `/${segment}`,
);
visibilityByPath.set(normalizedPath, visibility);
visibilityByDataPath.set(normalizedPath, visibility);
}
return true;
};
@@ -80,27 +81,30 @@ export function compileConfigSchemas(
const merged = mergeConfigSchemas(schemas.map(_ => _.value));
const validate = ajv.compile(merged);
const visibilityBySchemaPath = new Map<string, ConfigVisibility>();
traverse(merged, (schema, path) => {
if (schema.visibility && schema.visibility !== 'backend') {
visibilityBySchemaPath.set(path, schema.visibility);
}
});
return configs => {
const config = ConfigReader.fromConfigs(configs).get();
visibilityByPath.clear();
visibilityByDataPath.clear();
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}`;
}),
visibilityByPath: new Map(),
errors: validate.errors ?? [],
visibilityByDataPath: new Map(visibilityByDataPath),
visibilityBySchemaPath,
};
}
return {
visibilityByPath: new Map(visibilityByPath),
visibilityByDataPath: new Map(visibilityByDataPath),
visibilityBySchemaPath,
};
};
}
packages/config-loader/src/lib/schema/filtering.test.ts
+193 -1
View File
@@ -16,7 +16,7 @@
import { JsonObject } from '@backstage/config';
import { ConfigVisibility } from './types';
import { filterByVisibility } from './filtering';
import { filterByVisibility, filterErrorsByVisibility } from './filtering';
const data = {
arr: ['f', 'b', 's'],
@@ -175,3 +175,195 @@ describe('filterByVisibility', () => {
).toEqual(expected);
});
});
describe('filterErrorsByVisibility', () => {
it('should allow empty input', () => {
expect(
filterErrorsByVisibility(undefined, ['frontend'], new Map(), new Map()),
).toEqual([]);
expect(
filterErrorsByVisibility(
['my-error' as any],
undefined,
new Map(),
new Map(),
),
).toEqual(['my-error']);
expect(
filterErrorsByVisibility([], ['frontend'], new Map(), new Map()),
).toEqual([]);
});
it('should filter generic errors', () => {
const errors = [
{
keyword: 'something',
dataPath: '/a',
schemaPath: '#/properties/a/something',
params: {},
message: 'a',
},
{
keyword: 'something',
dataPath: '/b',
schemaPath: '#/properties/b/something',
params: {},
message: 'b',
},
{
keyword: 'something',
dataPath: '/c',
schemaPath: '#/properties/c/something',
params: {},
message: 'c',
},
];
const visibilityByDataPath = new Map<string, ConfigVisibility>([
['/a', 'frontend'],
['/c', 'secret'],
]);
expect(
filterErrorsByVisibility(
errors,
undefined,
visibilityByDataPath,
new Map(),
),
).toEqual([
expect.objectContaining({ message: 'a' }),
expect.objectContaining({ message: 'b' }),
expect.objectContaining({ message: 'c' }),
]);
expect(
filterErrorsByVisibility(
errors,
['frontend'],
visibilityByDataPath,
new Map(),
),
).toEqual([expect.objectContaining({ message: 'a' })]);
expect(
filterErrorsByVisibility(
errors,
['backend'],
visibilityByDataPath,
new Map(),
),
).toEqual([expect.objectContaining({ message: 'b' })]);
expect(
filterErrorsByVisibility(
errors,
['secret'],
visibilityByDataPath,
new Map(),
),
).toEqual([expect.objectContaining({ message: 'c' })]);
expect(
filterErrorsByVisibility(errors, [], visibilityByDataPath, new Map()),
).toEqual([]);
});
it('should always forward structural type errors', () => {
const errors = [
{
keyword: 'type',
dataPath: '/a',
schemaPath: '#/properties/a/type',
params: { type: 'number' },
message: 'a',
},
{
keyword: 'type',
dataPath: '/b',
schemaPath: '#/properties/b/type',
params: { type: 'string' },
message: 'b',
},
{
keyword: 'type',
dataPath: '/c',
schemaPath: '#/properties/c/type',
params: { type: 'array' },
message: 'c',
},
{
keyword: 'type',
dataPath: '/c',
schemaPath: '#/properties/c/type',
params: { type: 'object' },
message: 'd',
},
{
keyword: 'type',
dataPath: '/c',
schemaPath: '#/properties/c/type',
params: { type: 'null' },
message: 'e',
},
];
const visibilityByDataPath = new Map<string, ConfigVisibility>([
['/a', 'secret'],
['/b', 'secret'],
['/c', 'secret'],
['/d', 'secret'],
['/e', 'secret'],
]);
expect(
filterErrorsByVisibility(
errors,
['frontend'],
visibilityByDataPath,
new Map(),
),
).toEqual([
expect.objectContaining({ message: 'c' }),
expect.objectContaining({ message: 'd' }),
]);
});
it('should filter requirement errors based on schema path', () => {
const errors = [
{
keyword: 'required',
dataPath: '/a',
schemaPath: '#/properties/o/required',
params: { missingProperty: 'a' },
message: 'a',
},
{
keyword: 'required',
dataPath: '/b',
schemaPath: '#/properties/o/required',
params: { missingProperty: 'b' },
message: 'b',
},
{
keyword: 'required',
dataPath: '/c',
schemaPath: '#/properties/o/required',
params: { missingProperty: 'c' },
message: 'c',
},
];
const visibilityBySchemaPath = new Map<string, ConfigVisibility>([
['/properties/o', 'secret'],
['/properties/o/properties/a', 'frontend'],
['/properties/o/properties/b', 'secret'],
['/properties/o/properties/c/properties/x', 'frontend'],
]);
expect(
filterErrorsByVisibility(
errors,
['frontend'],
new Map(),
visibilityBySchemaPath,
),
).toEqual([
expect.objectContaining({ message: 'a' }),
expect.objectContaining({ message: 'c' }),
]);
});
});
packages/config-loader/src/lib/schema/filtering.ts
+54 -2
View File
@@ -19,6 +19,7 @@ import {
ConfigVisibility,
DEFAULT_CONFIG_VISIBILITY,
TransformFunc,
ValidationError,
} from './types';
/**
@@ -28,7 +29,7 @@ import {
export function filterByVisibility(
data: JsonObject,
includeVisibilities: ConfigVisibility[],
visibilityByPath: Map<string, ConfigVisibility>,
visibilityByDataPath: Map<string, ConfigVisibility>,
transformFunc?: TransformFunc<number | string | boolean>,
withFilteredKeys?: boolean,
): { data: JsonObject; filteredKeys?: string[] } {
@@ -40,7 +41,7 @@ export function filterByVisibility(
filterPath: string, // Matches the format of the ConfigReader
): JsonValue | undefined {
const visibility =
visibilityByPath.get(visibilityPath) ?? DEFAULT_CONFIG_VISIBILITY;
visibilityByDataPath.get(visibilityPath) ?? DEFAULT_CONFIG_VISIBILITY;
const isVisible = includeVisibilities.includes(visibility);
if (typeof jsonVal !== 'object') {
@@ -105,3 +106,54 @@ export function filterByVisibility(
data: (transform(data, '', '') as JsonObject) ?? {},
};
}
export function filterErrorsByVisibility(
errors: ValidationError[] | undefined,
includeVisibilities: ConfigVisibility[] | undefined,
visibilityByDataPath: Map<string, ConfigVisibility>,
visibilityBySchemaPath: Map<string, ConfigVisibility>,
): 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);
const fullPath = `${trimmedPath}/properties/${error.params.missingProperty}`;
if (
visibleSchemaPaths.some(visiblePath => visiblePath.startsWith(fullPath))
) {
return true;
}
}
const vis =
visibilityByDataPath.get(error.dataPath) ?? DEFAULT_CONFIG_VISIBILITY;
return vis && includeVisibilities.includes(vis);
});
}
packages/config-loader/src/lib/schema/load.test.ts
+142
View File
@@ -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",
);
});
});
packages/config-loader/src/lib/schema/load.ts
+25 -9
View File
@@ -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;
@@ -83,7 +99,7 @@ export async function loadConfigSchema(
...filterByVisibility(
data,
visibility,
result.visibilityByPath,
result.visibilityByDataPath,
valueTransform,
withFilteredKeys,
),
@@ -94,7 +110,7 @@ export async function loadConfigSchema(
...filterByVisibility(
data,
Array.from(CONFIG_VISIBILITIES),
result.visibilityByPath,
result.visibilityByDataPath,
valueTransform,
withFilteredKeys,
),
packages/config-loader/src/lib/schema/types.ts
+16 -2
View File
@@ -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<string, any>;
propertyName?: string;
message?: string;
};
/**
* The result of validating configuration data using a schema.
@@ -65,7 +72,14 @@ type ValidationResult = {
*
* The path in the key uses the form `/<key>/<sub-key>/<array-index>/<leaf-key>`
*/
visibilityByPath: Map<string, ConfigVisibility>;
visibilityByDataPath: Map<string, ConfigVisibility>;
/**
* The configuration visibilities that were discovered during validation.
*
* The path in the key uses the form `/properties/<key>/items/additionalProperties/<leaf-key>`
*/
visibilityBySchemaPath: Map<string, ConfigVisibility>;
};
/**