From 7a38a31699082338a3218b1cb73b7ca09ddbf94d Mon Sep 17 00:00:00 2001 From: sblausten Date: Tue, 20 Dec 2022 08:56:22 +0100 Subject: [PATCH 1/8] Do not fail fast on run checks endpoint when a single check fails and return error message alongside any results that completed Signed-off-by: sblausten --- .changeset/proud-birds-worry.md | 5 ++++ .../src/service/router.ts | 24 +++++++++++++++---- 2 files changed, 24 insertions(+), 5 deletions(-) create mode 100644 .changeset/proud-birds-worry.md diff --git a/.changeset/proud-birds-worry.md b/.changeset/proud-birds-worry.md new file mode 100644 index 0000000000..cef7120fe1 --- /dev/null +++ b/.changeset/proud-birds-worry.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-tech-insights-backend': patch +--- + +Complete check results run when a single check errors so that we don't block other checks from working due to an error in a single check diff --git a/plugins/tech-insights-backend/src/service/router.ts b/plugins/tech-insights-backend/src/service/router.ts index 6dba290422..d9cfa9b0ad 100644 --- a/plugins/tech-insights-backend/src/service/router.ts +++ b/plugins/tech-insights-backend/src/service/router.ts @@ -103,16 +103,30 @@ export async function createRouter< checks, entities, }: { checks: string[]; entities: CompoundEntityRef[] } = req.body; + let error = undefined; const tasks = entities.map(async entity => { const entityTriplet = typeof entity === 'string' ? entity : stringifyEntityRef(entity); - const results = await factChecker.runChecks(entityTriplet, checks); - return { - entity: entityTriplet, - results, - }; + try { + const results = await factChecker.runChecks(entityTriplet, checks); + return { + entity: entityTriplet, + results, + }; + } catch (e: any) { + const errorMessage = `Failed to run check for entity ${entityTriplet} due to error: ${e.message}`; + logger.error(errorMessage); + error = errorMessage; + return { + entity: entityTriplet, + results: [], + }; + } }); const results = await Promise.all(tasks); + if (error) { + return res.status(500).send({ error, results }); + } return res.json(results); }); } else { From 4d1094bef43a2a62180878f1d75239128bd4f8ac Mon Sep 17 00:00:00 2001 From: sblausten Date: Tue, 20 Dec 2022 11:45:33 +0100 Subject: [PATCH 2/8] Return all errors Signed-off-by: sblausten --- plugins/tech-insights-backend/src/service/router.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/tech-insights-backend/src/service/router.ts b/plugins/tech-insights-backend/src/service/router.ts index d9cfa9b0ad..e09114d2c7 100644 --- a/plugins/tech-insights-backend/src/service/router.ts +++ b/plugins/tech-insights-backend/src/service/router.ts @@ -103,7 +103,7 @@ export async function createRouter< checks, entities, }: { checks: string[]; entities: CompoundEntityRef[] } = req.body; - let error = undefined; + const errors: string[] = []; const tasks = entities.map(async entity => { const entityTriplet = typeof entity === 'string' ? entity : stringifyEntityRef(entity); @@ -116,7 +116,7 @@ export async function createRouter< } catch (e: any) { const errorMessage = `Failed to run check for entity ${entityTriplet} due to error: ${e.message}`; logger.error(errorMessage); - error = errorMessage; + errors.push(errorMessage); return { entity: entityTriplet, results: [], @@ -124,8 +124,8 @@ export async function createRouter< } }); const results = await Promise.all(tasks); - if (error) { - return res.status(500).send({ error, results }); + if (errors.length > 0) { + return res.status(500).send({ errors, results }); } return res.json(results); }); From 138b37994a7d44ae5466bd447411ba1174c227b4 Mon Sep 17 00:00:00 2001 From: sblausten Date: Tue, 20 Dec 2022 11:57:22 +0100 Subject: [PATCH 3/8] Return 200 unless all fail Signed-off-by: sblausten --- plugins/tech-insights-backend/src/service/router.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/plugins/tech-insights-backend/src/service/router.ts b/plugins/tech-insights-backend/src/service/router.ts index e09114d2c7..969f2974d6 100644 --- a/plugins/tech-insights-backend/src/service/router.ts +++ b/plugins/tech-insights-backend/src/service/router.ts @@ -124,9 +124,14 @@ export async function createRouter< } }); const results = await Promise.all(tasks); - if (errors.length > 0) { + const noResults = + results.length === 0 || results.flatMap(r => r.results).length === 0; + if (errors.length > 0 && noResults) { return res.status(500).send({ errors, results }); } + if (errors.length > 0) { + return res.json({ errors, results }); + } return res.json(results); }); } else { From a5ea62555092115148b475d87c73a0513a7f2c6d Mon Sep 17 00:00:00 2001 From: sblausten Date: Tue, 20 Dec 2022 12:00:42 +0100 Subject: [PATCH 4/8] Small tweak Signed-off-by: sblausten --- .../tech-insights-backend/src/service/router.test.ts | 12 ++++++++++++ plugins/tech-insights-backend/src/service/router.ts | 3 +-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/plugins/tech-insights-backend/src/service/router.test.ts b/plugins/tech-insights-backend/src/service/router.test.ts index 6935338a56..c8fe4de80e 100644 --- a/plugins/tech-insights-backend/src/service/router.test.ts +++ b/plugins/tech-insights-backend/src/service/router.test.ts @@ -101,6 +101,18 @@ describe('Tech Insights router tests', () => { }); }); + describe('/checks/run', () => { + it('should continue to run all checks if a single one fails', async () => { + await request(app) + .post('/checks/run') + .query({ + entity: 'a:a/a', + ids: ['check1', 'check2'], + }) + .expect(500); + }); + }); + describe('/facts/latest', () => { it('should be able to parse id request params for fact retrieval', async () => { await request(app) diff --git a/plugins/tech-insights-backend/src/service/router.ts b/plugins/tech-insights-backend/src/service/router.ts index 969f2974d6..1317687e68 100644 --- a/plugins/tech-insights-backend/src/service/router.ts +++ b/plugins/tech-insights-backend/src/service/router.ts @@ -124,8 +124,7 @@ export async function createRouter< } }); const results = await Promise.all(tasks); - const noResults = - results.length === 0 || results.flatMap(r => r.results).length === 0; + const noResults = results.flatMap(r => r.results).length === 0; if (errors.length > 0 && noResults) { return res.status(500).send({ errors, results }); } From 98dd59a15804a86bb9566695a573291a4a8c7d54 Mon Sep 17 00:00:00 2001 From: sblausten Date: Tue, 20 Dec 2022 12:27:40 +0100 Subject: [PATCH 5/8] Remove test Signed-off-by: sblausten --- .../tech-insights-backend/src/service/router.test.ts | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/plugins/tech-insights-backend/src/service/router.test.ts b/plugins/tech-insights-backend/src/service/router.test.ts index c8fe4de80e..6935338a56 100644 --- a/plugins/tech-insights-backend/src/service/router.test.ts +++ b/plugins/tech-insights-backend/src/service/router.test.ts @@ -101,18 +101,6 @@ describe('Tech Insights router tests', () => { }); }); - describe('/checks/run', () => { - it('should continue to run all checks if a single one fails', async () => { - await request(app) - .post('/checks/run') - .query({ - entity: 'a:a/a', - ids: ['check1', 'check2'], - }) - .expect(500); - }); - }); - describe('/facts/latest', () => { it('should be able to parse id request params for fact retrieval', async () => { await request(app) From 8de258cb8fbe377704f7a9bdc7e9328bc05708df Mon Sep 17 00:00:00 2001 From: sblausten Date: Wed, 21 Dec 2022 10:56:10 +0100 Subject: [PATCH 6/8] Always return 200 Signed-off-by: sblausten --- plugins/tech-insights-backend/src/service/router.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/plugins/tech-insights-backend/src/service/router.ts b/plugins/tech-insights-backend/src/service/router.ts index 1317687e68..c711c64628 100644 --- a/plugins/tech-insights-backend/src/service/router.ts +++ b/plugins/tech-insights-backend/src/service/router.ts @@ -124,10 +124,6 @@ export async function createRouter< } }); const results = await Promise.all(tasks); - const noResults = results.flatMap(r => r.results).length === 0; - if (errors.length > 0 && noResults) { - return res.status(500).send({ errors, results }); - } if (errors.length > 0) { return res.json({ errors, results }); } From 28b417769e0d24b65d82b60a66d0346fd9f199a6 Mon Sep 17 00:00:00 2001 From: sblausten Date: Wed, 21 Dec 2022 18:18:42 +0100 Subject: [PATCH 7/8] Add error to each result Signed-off-by: sblausten --- plugins/tech-insights-backend/src/service/router.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/plugins/tech-insights-backend/src/service/router.ts b/plugins/tech-insights-backend/src/service/router.ts index c711c64628..de40a81396 100644 --- a/plugins/tech-insights-backend/src/service/router.ts +++ b/plugins/tech-insights-backend/src/service/router.ts @@ -32,6 +32,7 @@ import { stringifyEntityRef, } from '@backstage/catalog-model'; import { errorHandler } from '@backstage/backend-common'; +import { serializeError } from '@backstage/errors/dist'; /** * @public @@ -103,7 +104,6 @@ export async function createRouter< checks, entities, }: { checks: string[]; entities: CompoundEntityRef[] } = req.body; - const errors: string[] = []; const tasks = entities.map(async entity => { const entityTriplet = typeof entity === 'string' ? entity : stringifyEntityRef(entity); @@ -114,19 +114,16 @@ export async function createRouter< results, }; } catch (e: any) { - const errorMessage = `Failed to run check for entity ${entityTriplet} due to error: ${e.message}`; - logger.error(errorMessage); - errors.push(errorMessage); + const error = serializeError(e); + logger.error(`${error.name}: ${error.message}`); return { entity: entityTriplet, + error: error, results: [], }; } }); const results = await Promise.all(tasks); - if (errors.length > 0) { - return res.json({ errors, results }); - } return res.json(results); }); } else { From f31659e9d805909a52bd8147d9b1376317c7da4d Mon Sep 17 00:00:00 2001 From: sblausten Date: Fri, 23 Dec 2022 18:05:35 +0100 Subject: [PATCH 8/8] Fix import Signed-off-by: sblausten --- plugins/tech-insights-backend/src/service/router.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/tech-insights-backend/src/service/router.ts b/plugins/tech-insights-backend/src/service/router.ts index de40a81396..5e58df1b20 100644 --- a/plugins/tech-insights-backend/src/service/router.ts +++ b/plugins/tech-insights-backend/src/service/router.ts @@ -32,7 +32,7 @@ import { stringifyEntityRef, } from '@backstage/catalog-model'; import { errorHandler } from '@backstage/backend-common'; -import { serializeError } from '@backstage/errors/dist'; +import { serializeError } from '@backstage/errors'; /** * @public