diff --git a/plugins/tech-insights-backend-module-jsonfc/api-report.md b/plugins/tech-insights-backend-module-jsonfc/api-report.md index e7c612215c..8f6bf0cd6c 100644 --- a/plugins/tech-insights-backend-module-jsonfc/api-report.md +++ b/plugins/tech-insights-backend-module-jsonfc/api-report.md @@ -5,6 +5,7 @@ ```ts import { BooleanCheckResult } from '@backstage/plugin-tech-insights-common'; import { CheckResponse } from '@backstage/plugin-tech-insights-common'; +import { CheckValidationResponse } from '@backstage/plugin-tech-insights-common'; import { DynamicFactCallback } from 'json-rules-engine'; import { FactChecker } from '@backstage/plugin-tech-insights-common'; import { FactOptions } from 'json-rules-engine'; @@ -72,7 +73,7 @@ export class JsonRulesEngineFactChecker checks: string[], ): Promise; // (undocumented) - validate(check: TechInsightJsonRuleCheck): Promise; + validate(check: TechInsightJsonRuleCheck): Promise; } // @public diff --git a/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.test.ts b/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.test.ts index 549274cefa..124030a545 100644 --- a/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.test.ts +++ b/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.test.ts @@ -289,12 +289,16 @@ describe('JsonRulesEngineFactChecker', () => { describe('when validating checks', () => { it('should succeed on valid rules', async () => { - const isValid = await factChecker.validate(testChecks.simple[0]); - expect(isValid).toBeTruthy(); + const validationResponse = await factChecker.validate( + testChecks.simple[0], + ); + expect(validationResponse.valid).toBeTruthy(); }); it('should fail on broken rules', async () => { - const isValid = await factChecker.validate(testChecks.broken[0]); - expect(isValid).toBeFalsy(); + const validationResponse = await factChecker.validate( + testChecks.broken[0], + ); + expect(validationResponse.valid).toBeFalsy(); }); }); }); diff --git a/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts b/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts index cfc31cdffc..7e8cab8b7f 100644 --- a/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts +++ b/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts @@ -22,6 +22,8 @@ import { TechInsightCheckRegistry, FlatTechInsightFact, TechInsightsStore, + CheckValidationResponse, + CheckValidationError, } from '@backstage/plugin-tech-insights-common'; import { Engine, EngineResult, TopLevelCondition } from 'json-rules-engine'; import { DefaultCheckRegistry } from './CheckRegistry'; @@ -110,22 +112,31 @@ export class JsonRulesEngineFactChecker } } - async validate(check: TechInsightJsonRuleCheck): Promise { + async validate( + check: TechInsightJsonRuleCheck, + ): Promise { const ajv = new Ajv({ verbose: true }); const validator = ajv.compile(validationSchema); const isValidToSchema = validator(check.rule); if (check.type !== JSON_RULE_ENGINE_CHECK_TYPE) { - this.logger.warn( - 'Only ${JSON_RULE_ENGINE_CHECK_TYPE} checks can be registered to this fact checker', - ); - return false; + const msg = `Only ${JSON_RULE_ENGINE_CHECK_TYPE} checks can be registered to this fact checker`; + this.logger.warn(msg); + return { + valid: false, + message: msg, + }; } if (!isValidToSchema) { + const msg = 'Failed to to validate conditions against JSON schema'; this.logger.warn( 'Failed to to validate conditions against JSON schema', validator.errors, ); - return false; + return { + valid: false, + message: msg, + errors: validator.errors, + }; } const existingSchemas = await this.repository.getLatestSchemas( @@ -146,7 +157,17 @@ export class JsonRulesEngineFactChecker )}`, ); }); - return failedReferences.length === 0; + const valid = failedReferences.length === 0; + return { + valid, + ...(!valid + ? { + message: `Check is referencing missing values from fact schemas: ${failedReferences + .map(it => it.ref) + .join(',')}`, + } + : {}), + }; } getChecks(): Promise { @@ -156,13 +177,14 @@ export class JsonRulesEngineFactChecker async addCheck( check: TechInsightJsonRuleCheck, ): Promise { - if (!(await this.validate(check))) { - this.logger.warn( - `Check validation failed when adding check ${check.name} to check registry.`, - ); - throw new Error( - 'Failed to add check to rules engine. Validation failed.', - ); + const checkValidationResponse = await this.validate(check); + if (!checkValidationResponse.valid) { + const msg = `Check validation failed when adding check ${check.name} to check registry.`; + this.logger.warn(msg); + throw new CheckValidationError({ + message: checkValidationResponse.message || msg, + errors: checkValidationResponse.errors, + }); } return await this.checkRegistry.register(check); } diff --git a/plugins/tech-insights-common/api-report.md b/plugins/tech-insights-common/api-report.md index 40f5203fb8..3c599870c5 100644 --- a/plugins/tech-insights-common/api-report.md +++ b/plugins/tech-insights-common/api-report.md @@ -4,6 +4,7 @@ ```ts import { Config } from '@backstage/config'; +import { CustomErrorBase } from '@backstage/errors'; import { DateTime } from 'luxon'; import { Logger as Logger_2 } from 'winston'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; @@ -30,6 +31,28 @@ export type CheckResult = { check: CheckResponse; }; +// @public +export class CheckValidationError extends CustomErrorBase { + constructor({ + message, + cause, + errors, + }: { + message: string; + cause?: Error; + errors?: any; + }); + // (undocumented) + errors?: any; +} + +// @public +export type CheckValidationResponse = { + valid: boolean; + message?: string; + errors?: any; +}; + // @public export interface FactChecker< CheckType extends TechInsightCheck, @@ -38,7 +61,7 @@ export interface FactChecker< addCheck(check: CheckType): Promise; getChecks(): Promise; runChecks(entity: string, checks: string[]): Promise; - validate(check: CheckType): Promise; + validate(check: CheckType): Promise; } // @public diff --git a/plugins/tech-insights-common/package.json b/plugins/tech-insights-common/package.json index 37d66a9379..15ccc1a682 100644 --- a/plugins/tech-insights-common/package.json +++ b/plugins/tech-insights-common/package.json @@ -31,6 +31,7 @@ }, "dependencies": { "@backstage/backend-common": "^0.9.0", + "@backstage/errors": "^0.1.2", "@backstage/config": "^0.1.8", "@types/luxon": "^2.0.5", "luxon": "^2.0.2", diff --git a/plugins/tech-insights-common/src/checks.ts b/plugins/tech-insights-common/src/checks.ts index 02963ab624..062a5baaaa 100644 --- a/plugins/tech-insights-common/src/checks.ts +++ b/plugins/tech-insights-common/src/checks.ts @@ -15,6 +15,7 @@ */ import { TechInsightsStore } from './persistence'; import { CheckResponse, FactResponse } from './responses'; +import { CustomErrorBase } from '@backstage/errors'; /** * A factory wrapper to construct FactChecker implementations. @@ -82,7 +83,7 @@ export interface FactChecker< * @param check - The check to be validated * @returns - Validation result */ - validate(check: CheckType): Promise; + validate(check: CheckType): Promise; } /** @@ -171,3 +172,39 @@ export interface TechInsightCheck { */ failureMetadata?: Record; } + +/** + * Validation response from CheckValidator + * + * May contain additional data for display purposes + * @public + */ +export type CheckValidationResponse = { + valid: boolean; + message?: string; + errors?: any; +}; + +/** + * Error object for Validation Errors + * + * Can be used to short circuit larger execution paths instead of passing validation response around + * + * @public + */ +export class CheckValidationError extends CustomErrorBase { + errors?: any; + + constructor({ + message, + cause, + errors, + }: { + message: string; + cause?: Error; + errors?: any; + }) { + super(message, cause); + this.errors = errors; + } +}