config-loader: filter out schema errors that aren't visible

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>

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(),
     });

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,
       };
     }
 

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<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);
+      if (
+        visibleSchemaPaths.some(visiblePath =>
+          visiblePath.startsWith(trimmedPath),
+        )
+      ) {
+        return true;
+      }
+    }
+
+    const vis = visibilityByDataPath.get(error.dataPath);
+    return vis && includeVisibilities.includes(vis);
+  });
+}

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",
+    );
+  });
 });

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;

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<string, any>;
+  propertyName?: string;
+  message?: string;
+};
 
 /**
  * The result of validating configuration data using a schema.