Modify validation to have a more user friendly API

* Adding a container type to be returned from validate function
* Modifying JsonRulesEngineFactChecker to throw with error information if check addition fails on validation

Signed-off-by: Jussi Hallila <jussi@hallila.com>

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<JsonRuleBooleanCheckResult[]>;
   // (undocumented)
-  validate(check: TechInsightJsonRuleCheck): Promise<boolean>;
+  validate(check: TechInsightJsonRuleCheck): Promise<CheckValidationResponse>;
 }
 
 // @public

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

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<boolean> {
+  async validate(
+    check: TechInsightJsonRuleCheck,
+  ): Promise<CheckValidationResponse> {
     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<TechInsightJsonRuleCheck[]> {
@@ -156,13 +177,14 @@ export class JsonRulesEngineFactChecker
   async addCheck(
     check: TechInsightJsonRuleCheck,
   ): Promise<TechInsightJsonRuleCheck> {
-    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);
   }

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<CheckType>;
   getChecks(): Promise<CheckType[]>;
   runChecks(entity: string, checks: string[]): Promise<CheckResultType[]>;
-  validate(check: CheckType): Promise<boolean>;
+  validate(check: CheckType): Promise<CheckValidationResponse>;
 }
 
 // @public

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",

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<boolean>;
+  validate(check: CheckType): Promise<CheckValidationResponse>;
 }
 
 /**
@@ -171,3 +172,39 @@ export interface TechInsightCheck {
    */
   failureMetadata?: Record<string, any>;
 }
+
+/**
+ * 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;
+  }
+}