From c88d8339a3c7ed3d088cc5491e340653cae1bbec Mon Sep 17 00:00:00 2001 From: Jacob Wejendorp Date: Thu, 16 Dec 2021 14:40:31 +0100 Subject: [PATCH 1/6] fix: allow partial evaluation of checks in jsonfc factchecker This change should fix the entity check endpoint when some facts are missing, e.g. the retriever has not run yet. Signed-off-by: Jacob Wejendorp --- .../src/service/JsonRulesEngineFactChecker.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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 ae1f18a373..44087698ab 100644 --- a/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts +++ b/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts @@ -88,7 +88,13 @@ export class JsonRulesEngineFactChecker techInsightChecks.forEach(techInsightCheck => { const rule = techInsightCheck.rule; rule.name = techInsightCheck.id; - engine.addRule({ ...techInsightCheck.rule, event: noopEvent }); + // Only run checks that have all the facts available: + const hasAllFacts = techInsightCheck.factIds.every( + retrieverId => !!facts[retrieverId], + ); + if (hasAllFacts) { + engine.addRule({ ...techInsightCheck.rule, event: noopEvent }); + } }); const factValues = Object.values(facts).reduce( (acc, it) => ({ ...acc, ...it.facts }), From cf7fc2b41a8ae84c26f96ce7ea2f9e053bc23afa Mon Sep 17 00:00:00 2001 From: Jacob Wejendorp Date: Thu, 16 Dec 2021 14:52:03 +0100 Subject: [PATCH 2/6] fix: handle empty checks in tech-insights scorecard Show a warning about missing data instead of crashing if checks return empty. Signed-off-by: Jacob Wejendorp --- .../src/components/ScorecardsOverview/ChecksOverview.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugins/tech-insights/src/components/ScorecardsOverview/ChecksOverview.tsx b/plugins/tech-insights/src/components/ScorecardsOverview/ChecksOverview.tsx index 10025d1ba6..b07eeab339 100644 --- a/plugins/tech-insights/src/components/ScorecardsOverview/ChecksOverview.tsx +++ b/plugins/tech-insights/src/components/ScorecardsOverview/ChecksOverview.tsx @@ -21,6 +21,7 @@ import { Content, Page, InfoCard } from '@backstage/core-components'; import { CheckResult } from '@backstage/plugin-tech-insights-common'; import { techInsightsApiRef } from '../../api/TechInsightsApi'; import { BackstageTheme } from '@backstage/theme'; +import { Alert } from '@material-ui/lab'; const useStyles = makeStyles((theme: BackstageTheme) => ({ contentScorecards: { @@ -40,6 +41,9 @@ type Checks = { export const ChecksOverview = ({ checks }: Checks) => { const classes = useStyles(); const api = useApi(techInsightsApiRef); + if (!checks.length) { + return No checks have any data yet.; + } const checkRenderType = api.getScorecardsDefinition( checks[0].check.type, checks, From b8d7602cfaf3e0e6cb84487caa0e5dde2c4dbe2c Mon Sep 17 00:00:00 2001 From: Jacob Wejendorp Date: Thu, 16 Dec 2021 15:41:58 +0100 Subject: [PATCH 3/6] test: add tests for JsonRulesEngineFactChecker changes Signed-off-by: Jacob Wejendorp --- .../JsonRulesEngineFactChecker.test.ts | 66 +++++++++++++------ 1 file changed, 45 insertions(+), 21 deletions(-) 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 77b3e6c8b1..8d5e9674ca 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 @@ -52,6 +52,26 @@ const testChecks: Record = { name: 'brokenTestCheck2', type: JSON_RULE_ENGINE_CHECK_TYPE, description: 'Second Broken Check For Testing', + factIds: ['test-factretriever'], + rule: { + conditions: { + any: [ + { + fact: 'somefact', + operator: 'lessThan', + value: 1, + }, + ], + }, + }, + }, + ], + brokennotfound: [ + { + id: 'brokenTestCheckNotFound', + name: 'brokenTestCheckNotFound', + type: JSON_RULE_ENGINE_CHECK_TYPE, + description: 'Third Broken Check For Testing', factIds: ['non-existing-factretriever'], rule: { conditions: { @@ -122,7 +142,16 @@ const latestSchemasMock = jest.fn().mockImplementation(() => [ }, ]); const factsBetweenTimestampsByIdsMock = jest.fn(); -const latestFactsByIdsMock = jest.fn().mockImplementation(() => ({})); +const latestFactsByIdsMock = jest.fn().mockImplementation(() => + Promise.resolve({ + ['test-factretriever']: { + id: 'test-factretriever', + facts: { + testnumberfact: 3, + }, + }, + }), +); const mockCheckRegistry = { getAll(checks: string[]) { return checks.flatMap(check => testChecks[check]); @@ -156,17 +185,22 @@ describe('JsonRulesEngineFactChecker', () => { 'Failed to run rules engine, Undefined fact: somefact', ); }); - it('should respond with result, facts, fact schemas and checks', async () => { - latestFactsByIdsMock.mockImplementation(() => - Promise.resolve({ - ['test-factretriever']: { - id: 'test-factretriever', - facts: { - testnumberfact: 3, - }, - }, + + it('should skip checks where fact data is missing', async () => { + const skipped = async () => + await factChecker.runChecks('a/a/a', ['brokennotfound']); + await expect(skipped()).resolves.toEqual([]); + + const partial = async () => + await factChecker.runChecks('a/a/a', ['brokennotfound', 'simple']); + await expect(partial()).resolves.toEqual([ + expect.objectContaining({ + check: expect.objectContaining({ id: 'simpleTestCheck' }), }), - ); + ]); + }); + + it('should respond with result, facts, fact schemas and checks', async () => { const results = await factChecker.runChecks('a/a/a', ['simple']); expect(results).toHaveLength(1); expect(results[0]).toMatchObject({ @@ -203,16 +237,6 @@ describe('JsonRulesEngineFactChecker', () => { }); it('should gracefully handle multiple check at once', async () => { - latestFactsByIdsMock.mockImplementation(() => - Promise.resolve({ - ['test-factretriever']: { - id: 'test-factretriever', - facts: { - testnumberfact: 3, - }, - }, - }), - ); const results = await factChecker.runChecks('a/a/a', [ 'simple', 'simple2', From 6ff4408fa694afa00a137b4d9efe343db94d5bba Mon Sep 17 00:00:00 2001 From: Jacob Wejendorp Date: Thu, 16 Dec 2021 15:26:43 +0100 Subject: [PATCH 4/6] chore: add changeset to tech-insights changes Signed-off-by: Jacob Wejendorp --- .changeset/friendly-kids-mix.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changeset/friendly-kids-mix.md diff --git a/.changeset/friendly-kids-mix.md b/.changeset/friendly-kids-mix.md new file mode 100644 index 0000000000..b59209290c --- /dev/null +++ b/.changeset/friendly-kids-mix.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-tech-insights': patch +'@backstage/plugin-tech-insights-backend-module-jsonfc': patch +--- + +RunChecks endpoint now handles missing retriever data in checks. Instead of +showing server errors, the checks will be shown for checks whose retrievers have +data, and a warning will be shown if no checks are returned. From 91c353da3a60023ab5a345730956b5256e3b6760 Mon Sep 17 00:00:00 2001 From: Jacob Wejendorp Date: Thu, 16 Dec 2021 17:26:53 +0100 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: Marley <55280588+marleypowell@users.noreply.github.com> Signed-off-by: Jacob Wejendorp --- .../service/JsonRulesEngineFactChecker.test.ts | 16 +++++++--------- .../src/service/JsonRulesEngineFactChecker.ts | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) 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 8d5e9674ca..5c33e9e369 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 @@ -142,16 +142,14 @@ const latestSchemasMock = jest.fn().mockImplementation(() => [ }, ]); const factsBetweenTimestampsByIdsMock = jest.fn(); -const latestFactsByIdsMock = jest.fn().mockImplementation(() => - Promise.resolve({ - ['test-factretriever']: { - id: 'test-factretriever', - facts: { - testnumberfact: 3, - }, +const latestFactsByIdsMock = jest.fn().mockResolvedValue({ + ['test-factretriever']: { + id: 'test-factretriever', + facts: { + testnumberfact: 3, }, - }), -); + }, +}); const mockCheckRegistry = { getAll(checks: string[]) { return checks.flatMap(check => testChecks[check]); 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 44087698ab..640ad51113 100644 --- a/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts +++ b/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts @@ -90,7 +90,7 @@ export class JsonRulesEngineFactChecker rule.name = techInsightCheck.id; // Only run checks that have all the facts available: const hasAllFacts = techInsightCheck.factIds.every( - retrieverId => !!facts[retrieverId], + factId => !!facts[factId], ); if (hasAllFacts) { engine.addRule({ ...techInsightCheck.rule, event: noopEvent }); From 8814c893e3bd3efde71a3efac32c844d661f7472 Mon Sep 17 00:00:00 2001 From: Jacob Wejendorp Date: Fri, 17 Dec 2021 11:59:29 +0100 Subject: [PATCH 6/6] feat: add logging when skipping checks Signed-off-by: Jacob Wejendorp --- .../src/service/JsonRulesEngineFactChecker.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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 640ad51113..2623a5c2ee 100644 --- a/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts +++ b/plugins/tech-insights-backend-module-jsonfc/src/service/JsonRulesEngineFactChecker.ts @@ -90,10 +90,18 @@ export class JsonRulesEngineFactChecker rule.name = techInsightCheck.id; // Only run checks that have all the facts available: const hasAllFacts = techInsightCheck.factIds.every( - factId => !!facts[factId], + factId => facts[factId], ); if (hasAllFacts) { engine.addRule({ ...techInsightCheck.rule, event: noopEvent }); + } else { + this.logger.warn( + `Skipping ${ + rule.name + } due to missing facts: ${techInsightCheck.factIds + .filter(factId => !facts[factId]) + .join(', ')}`, + ); } }); const factValues = Object.values(facts).reduce(