From b2363f3e3b4a54968bc76c1b5429a8fb32ce4d5d Mon Sep 17 00:00:00 2001 From: Jussi Hallila Date: Thu, 21 Oct 2021 09:25:26 +0200 Subject: [PATCH] Add explicit type information to checks Types can be used to: * Determine which check logic to run * Determine the correct persistence option to use * Choose correct ser/deser logic for the check * Determine correct components to render on the frontend Modify router API to be post for better usability. The API might end up doing writes if caching etc. is added in later as well. Modify CheckRegistry & FactChecker to return persisted check when it is registered. Signed-off-by: Jussi Hallila --- .../README.md | 2 ++ .../api-report.md | 5 ++++- .../src/constants.ts | 20 +++++++++++++++++++ .../src/index.ts | 1 + .../src/service/CheckRegistry.ts | 3 ++- .../JsonRulesEngineFactChecker.test.ts | 9 ++++++++- .../src/service/JsonRulesEngineFactChecker.ts | 19 ++++++++++++++---- .../src/service/router.ts | 17 ++++++++++++---- plugins/tech-insights-common/api-report.md | 8 ++++---- plugins/tech-insights-common/src/checks.ts | 18 +++++++++++++++-- plugins/tech-insights-common/src/responses.ts | 6 ++++++ 11 files changed, 91 insertions(+), 17 deletions(-) create mode 100644 plugins/tech-insights-backend-module-jsonfc/src/constants.ts diff --git a/plugins/tech-insights-backend-module-jsonfc/README.md b/plugins/tech-insights-backend-module-jsonfc/README.md index ea45a1b4b0..99570f1c7a 100644 --- a/plugins/tech-insights-backend-module-jsonfc/README.md +++ b/plugins/tech-insights-backend-module-jsonfc/README.md @@ -52,10 +52,12 @@ Checks for this FactChecker are constructed as `json-rules-engine` compatible JS ```ts import { TechInsightJsonRuleCheck } from '../types'; +import { JSON_RULE_ENGINE_CHECK_TYPE } from '../constants'; export const exampleCheck: TechInsightJsonRuleCheck = { id: 'demodatacheck', // Unique identifier of this check name: 'demodatacheck', // A human readable name of this check to be displayed in the UI + type: JSON_RULE_ENGINE_CHECK_TYPE, // Type identifier of the check. Used to run logic against, determine persistence option to use and render correct components on the UI description: 'A fact check for demoing purposes', // A description to be displayed in the UI factRefs: ['demo-poc.factretriever'], // References to fact containers that this check uses. See documentation on FactRetrievers for more information on these rule: { diff --git a/plugins/tech-insights-backend-module-jsonfc/api-report.md b/plugins/tech-insights-backend-module-jsonfc/api-report.md index bf08028a00..e7c612215c 100644 --- a/plugins/tech-insights-backend-module-jsonfc/api-report.md +++ b/plugins/tech-insights-backend-module-jsonfc/api-report.md @@ -33,6 +33,9 @@ export interface DynamicFact { options?: FactOptions; } +// @public (undocumented) +export const JSON_RULE_ENGINE_CHECK_TYPE = 'json-rules-engine'; + // @public (undocumented) export interface JsonRuleBooleanCheckResult extends BooleanCheckResult { // (undocumented) @@ -60,7 +63,7 @@ export class JsonRulesEngineFactChecker checkRegistry, }: JsonRulesEngineFactCheckerOptions); // (undocumented) - addCheck(check: TechInsightJsonRuleCheck): Promise; + addCheck(check: TechInsightJsonRuleCheck): Promise; // (undocumented) getChecks(): Promise; // (undocumented) diff --git a/plugins/tech-insights-backend-module-jsonfc/src/constants.ts b/plugins/tech-insights-backend-module-jsonfc/src/constants.ts new file mode 100644 index 0000000000..5fd69a3fef --- /dev/null +++ b/plugins/tech-insights-backend-module-jsonfc/src/constants.ts @@ -0,0 +1,20 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @public + */ +export const JSON_RULE_ENGINE_CHECK_TYPE = 'json-rules-engine'; diff --git a/plugins/tech-insights-backend-module-jsonfc/src/index.ts b/plugins/tech-insights-backend-module-jsonfc/src/index.ts index a2561eaa3b..1a537b8257 100644 --- a/plugins/tech-insights-backend-module-jsonfc/src/index.ts +++ b/plugins/tech-insights-backend-module-jsonfc/src/index.ts @@ -17,6 +17,7 @@ export { JsonRulesEngineFactCheckerFactory, JsonRulesEngineFactChecker, } from './service/JsonRulesEngineFactChecker'; +export { JSON_RULE_ENGINE_CHECK_TYPE } from './constants'; export type { JsonRulesEngineFactCheckerFactoryOptions, JsonRulesEngineFactCheckerOptions, diff --git a/plugins/tech-insights-backend-module-jsonfc/src/service/CheckRegistry.ts b/plugins/tech-insights-backend-module-jsonfc/src/service/CheckRegistry.ts index 2ae1ac0b83..016fb05b31 100644 --- a/plugins/tech-insights-backend-module-jsonfc/src/service/CheckRegistry.ts +++ b/plugins/tech-insights-backend-module-jsonfc/src/service/CheckRegistry.ts @@ -31,13 +31,14 @@ export class DefaultCheckRegistry }); } - async register(check: CheckType) { + async register(check: CheckType): Promise { if (this.checks.has(check.id)) { throw new ConflictError( `Tech insight check with id ${check.id} has already been registered`, ); } this.checks.set(check.id, check); + return check; } async get(checkId: string): Promise { 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 d574e13c5f..549274cefa 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 @@ -18,7 +18,10 @@ import { TechInsightCheckRegistry, TechInsightsStore, } from '@backstage/plugin-tech-insights-common'; -import { JsonRulesEngineFactCheckerFactory } from '../index'; +import { + JSON_RULE_ENGINE_CHECK_TYPE, + JsonRulesEngineFactCheckerFactory, +} from '../index'; import { getVoidLogger } from '@backstage/backend-common'; import { TechInsightJsonRuleCheck } from '../types'; @@ -27,6 +30,7 @@ const testChecks: Record = { { id: 'brokenTestCheck', name: 'brokenTestCheck', + type: JSON_RULE_ENGINE_CHECK_TYPE, description: 'Broken Check For Testing', factRefs: ['test-factretriever'], rule: { @@ -46,6 +50,7 @@ const testChecks: Record = { { id: 'brokenTestCheck2', name: 'brokenTestCheck2', + type: JSON_RULE_ENGINE_CHECK_TYPE, description: 'Second Broken Check For Testing', factRefs: ['non-existing-factretriever'], rule: { @@ -65,6 +70,7 @@ const testChecks: Record = { { id: 'simpleTestCheck', name: 'simpleTestCheck', + type: JSON_RULE_ENGINE_CHECK_TYPE, description: 'Simple Check For Testing', factRefs: ['test-factretriever'], rule: { @@ -85,6 +91,7 @@ const testChecks: Record = { { id: 'simpleTestCheck2', name: 'simpleTestCheck2', + type: JSON_RULE_ENGINE_CHECK_TYPE, description: 'Second Simple Check For Testing', factRefs: ['test-factretriever'], rule: { 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 964575b494..cfc31cdffc 100644 --- a/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts +++ b/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts @@ -29,6 +29,7 @@ import { Logger } from 'winston'; import { pick } from 'lodash'; import Ajv from 'ajv'; import * as validationSchema from './validation-schema.json'; +import { JSON_RULE_ENGINE_CHECK_TYPE } from '../constants'; const noopEvent = { type: 'noop', @@ -113,6 +114,12 @@ export class JsonRulesEngineFactChecker 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; + } if (!isValidToSchema) { this.logger.warn( 'Failed to to validate conditions against JSON schema', @@ -146,15 +153,18 @@ export class JsonRulesEngineFactChecker return this.checkRegistry.list(); } - async addCheck(check: TechInsightJsonRuleCheck): Promise { + async addCheck( + check: TechInsightJsonRuleCheck, + ): Promise { if (!(await this.validate(check))) { this.logger.warn( `Check validation failed when adding check ${check.name} to check registry.`, ); - return false; + throw new Error( + 'Failed to add check to rules engine. Validation failed.', + ); } - this.checkRegistry.register(check); - return true; + return await this.checkRegistry.register(check); } private retrieveFactReferences( @@ -216,6 +226,7 @@ export class JsonRulesEngineFactChecker ) { const returnable = { id: techInsightCheck.id, + type: techInsightCheck.type, name: techInsightCheck.name, description: techInsightCheck.description, factRefs: techInsightCheck.factRefs, diff --git a/plugins/tech-insights-backend/src/service/router.ts b/plugins/tech-insights-backend/src/service/router.ts index 3649f522dd..cc41868cd8 100644 --- a/plugins/tech-insights-backend/src/service/router.ts +++ b/plugins/tech-insights-backend/src/service/router.ts @@ -83,11 +83,16 @@ export async function createRouter< return res.send(await factChecker.getChecks()); }); - router.get('/checks/:namespace/:kind/:name', async (req, res) => { + router.post('/checks/run/:namespace/:kind/:name', async (req, res) => { const { namespace, kind, name } = req.params; - const checks = req.query.checks as string[]; - const entityTriplet = `${namespace.toLowerCase()}/${kind.toLowerCase()}/${name.toLowerCase()}`; try { + if (!('checks' in req.body)) { + return res.status(422).send({ + message: 'Failed to get checks from request.', + }); + } + const { checks }: { checks: string[] } = req.body; + const entityTriplet = `${namespace.toLowerCase()}/${kind.toLowerCase()}/${name.toLowerCase()}`; const checkResult = await factChecker.runChecks(entityTriplet, checks); return res.send(checkResult); } catch (e) { @@ -120,7 +125,11 @@ export async function createRouter< const startDatetime = DateTime.fromISO(req.query.startDatetime as string); const endDatetime = DateTime.fromISO(req.query.endDatetime as string); if (!startDatetime.isValid || !endDatetime.isValid) { - return res.status(422).send('Failed to parse datetime from request'); + return res.status(422).send({ + message: 'Failed to parse datetime from request', + field: !startDatetime.isValid ? 'startDateTime' : 'endDateTime', + value: !startDatetime.isValid ? startDatetime : endDatetime, + }); } const entityTriplet = `${namespace.toLowerCase()}/${kind.toLowerCase()}/${name.toLowerCase()}`; return res.send( diff --git a/plugins/tech-insights-common/api-report.md b/plugins/tech-insights-common/api-report.md index 6d0d40239b..40f5203fb8 100644 --- a/plugins/tech-insights-common/api-report.md +++ b/plugins/tech-insights-common/api-report.md @@ -21,6 +21,7 @@ export interface CheckResponse { id: string; metadata?: Record; name: string; + type: string; } // @public @@ -34,7 +35,7 @@ export interface FactChecker< CheckType extends TechInsightCheck, CheckResultType extends CheckResult, > { - addCheck(check: CheckType): Promise; + addCheck(check: CheckType): Promise; getChecks(): Promise; runChecks(entity: string, checks: string[]): Promise; validate(check: CheckType): Promise; @@ -108,14 +109,13 @@ export type FlatTechInsightFact = TechInsightFact & { // @public export interface TechInsightCheck { - // (undocumented) description: string; factRefs: string[]; failureMetadata?: Record; id: string; - // (undocumented) name: string; successMetadata?: Record; + type: string; } // @public @@ -127,7 +127,7 @@ export interface TechInsightCheckRegistry { // (undocumented) list(): Promise; // (undocumented) - register(check: CheckType): Promise; + register(check: CheckType): Promise; } // @public diff --git a/plugins/tech-insights-common/src/checks.ts b/plugins/tech-insights-common/src/checks.ts index eaa4792fe3..02963ab624 100644 --- a/plugins/tech-insights-common/src/checks.ts +++ b/plugins/tech-insights-common/src/checks.ts @@ -66,7 +66,7 @@ export interface FactChecker< * @param check - The actual check to be added. * @returns - An indicator if fact was successfully added */ - addCheck(check: CheckType): Promise; + addCheck(check: CheckType): Promise; /** * Retrieves all available checks that can be used to run checks against. @@ -93,7 +93,7 @@ export interface FactChecker< * */ export interface TechInsightCheckRegistry { - register(check: CheckType): Promise; + register(check: CheckType): Promise; get(checkId: string): Promise; getAll(checks: string[]): Promise; list(): Promise; @@ -135,7 +135,21 @@ export interface TechInsightCheck { */ id: string; + /** + * Type identifier for the check. + * Can be used to determine storage options, logical routing to correct FactChecker implementation + * or to help frontend render correct component types based on this + */ + type: string; + + /** + * Human readable name of the check, may be displayed in the UI + */ name: string; + + /** + * Human readable description of the check, may be displayed in the UI + */ description: string; /** diff --git a/plugins/tech-insights-common/src/responses.ts b/plugins/tech-insights-common/src/responses.ts index 983a68ee2f..0cd1d0a2e8 100644 --- a/plugins/tech-insights-common/src/responses.ts +++ b/plugins/tech-insights-common/src/responses.ts @@ -26,6 +26,12 @@ export interface CheckResponse { * Identifier of the Check */ id: string; + /** + * Type identifier for the check. + * Can be used to determine storage options, logical routing to correct FactChecker implementation + * or to help frontend render correct component types based on this + */ + type: string; /** * Human readable name of the Check */