From 54baecbd1a56d00de487cb11d828c8a9fd30bf5c Mon Sep 17 00:00:00 2001 From: Robert Bunning Date: Fri, 23 Dec 2022 16:26:33 -0500 Subject: [PATCH] Implement caching via response etags for adr backend Signed-off-by: Robert Bunning --- packages/backend/src/plugins/adr.ts | 6 +- .../adr-backend/src/service/router.test.ts | 30 +++++- plugins/adr-backend/src/service/router.ts | 96 ++++++++++++++++--- plugins/adr/src/api/AdrClient.ts | 2 +- 4 files changed, 116 insertions(+), 18 deletions(-) diff --git a/packages/backend/src/plugins/adr.ts b/packages/backend/src/plugins/adr.ts index dceb4d0c69..6fa0d72969 100644 --- a/packages/backend/src/plugins/adr.ts +++ b/packages/backend/src/plugins/adr.ts @@ -21,5 +21,9 @@ import { PluginEnvironment } from '../types'; export default async function createPlugin( env: PluginEnvironment, ): Promise { - return await createRouter(env.reader); + return await createRouter({ + reader: env.reader, + cacheClient: env.cache.getClient(), + logger: env.logger, + }); } diff --git a/plugins/adr-backend/src/service/router.test.ts b/plugins/adr-backend/src/service/router.test.ts index 76b0f5552d..74955fcc6a 100644 --- a/plugins/adr-backend/src/service/router.test.ts +++ b/plugins/adr-backend/src/service/router.test.ts @@ -15,6 +15,7 @@ */ import { + CacheClient, ReadTreeResponse, ReadTreeResponseFile, ReadUrlResponse, @@ -23,6 +24,7 @@ import { import express from 'express'; import request from 'supertest'; import { createRouter } from './router'; +import { Logger } from 'winston'; const listEndpointName = '/list'; const fileEndpointName = '/file'; @@ -90,13 +92,39 @@ const mockUrlReader: UrlReader = { }, }; +class MockCacheClient implements CacheClient { + private itemRegistry: { [key: string]: any }; + + constructor() { + this.itemRegistry = {}; + } + + async get(key: string) { + return this.itemRegistry[key]; + } + + async set(key: string, value: any) { + this.itemRegistry[key] = value; + } + + async delete(key: string) { + delete this.itemRegistry[key]; + } +} + describe('createRouter', () => { let app: express.Express; beforeEach(async () => { jest.resetAllMocks(); - const router = await createRouter(mockUrlReader); + const router = await createRouter({ + reader: mockUrlReader, + cacheClient: new MockCacheClient(), + logger: { + error: (message: any) => message, + } as Logger, + }); app = express().use(router); }); diff --git a/plugins/adr-backend/src/service/router.ts b/plugins/adr-backend/src/service/router.ts index 26bf381596..9c21b03d04 100644 --- a/plugins/adr-backend/src/service/router.ts +++ b/plugins/adr-backend/src/service/router.ts @@ -14,12 +14,24 @@ * limitations under the License. */ -import { UrlReader } from '@backstage/backend-common'; +import { CacheClient, UrlReader } from '@backstage/backend-common'; +import { NotModifiedError, stringifyError } from '@backstage/errors'; +import { Logger } from 'winston'; import express from 'express'; import Router from 'express-promise-router'; +export type AdrRouterOptions = { + reader: UrlReader; + cacheClient: CacheClient; + logger: Logger; +}; + /** @public */ -export async function createRouter(reader: UrlReader): Promise { +export async function createRouter( + options: AdrRouterOptions, +): Promise { + const { reader, cacheClient, logger } = options; + const router = Router(); router.use(express.json()); @@ -31,17 +43,46 @@ export async function createRouter(reader: UrlReader): Promise { return; } - const treeGetResponse = await reader.readTree(urlToProcess); - const files = await treeGetResponse.files(); - const fileData = files.map(file => { - return { - type: 'file', - name: file.path.substring(file.path.lastIndexOf('/') + 1), - path: file.path, - }; - }); + const cachedTree = (await cacheClient.get(urlToProcess)) as { + data: { + type: string; + name: string; + path: string; + }[]; + etag: string; + }; + const cachedData = cachedTree?.data; - res.json({ data: fileData }); + try { + const treeGetResponse = await reader.readTree(urlToProcess, { + etag: cachedTree?.etag, + }); + const files = await treeGetResponse.files(); + const data = files.map(file => { + return { + type: 'file', + name: file.path.substring(file.path.lastIndexOf('/') + 1), + path: file.path, + }; + }); + + await cacheClient.set(urlToProcess, { + data, + etag: treeGetResponse.etag, + }); + + res.json({ data }); + } catch (error: any) { + if (cachedData && error.name === NotModifiedError.name) { + res.json({ data: cachedData }); + return; + } + + const message = stringifyError(error); + logger.error(`Unable to fetch ADRs from ${urlToProcess}: ${message}`); + res.statusCode = 500; + res.json({ message }); + } }); router.get('/file', async (req, res) => { @@ -52,10 +93,35 @@ export async function createRouter(reader: UrlReader): Promise { return; } - const fileGetResponse = await reader.readUrl(urlToProcess); - const fileBuffer = await fileGetResponse.buffer(); + const cachedFileContent = (await cacheClient.get(urlToProcess)) as { + data: string; + etag: string; + }; - res.json({ data: fileBuffer.toString() }); + try { + const fileGetResponse = await reader.readUrl(urlToProcess, { + etag: cachedFileContent?.etag, + }); + const fileBuffer = await fileGetResponse.buffer(); + const data = fileBuffer.toString(); + + await cacheClient.set(urlToProcess, { + data, + etag: fileGetResponse.etag, + }); + + res.json({ data }); + } catch (error) { + if (cachedFileContent && error.name === NotModifiedError.name) { + res.json({ data: cachedFileContent.data }); + return; + } + + const message = stringifyError(error); + logger.error(`Unable to fetch ADRs from ${urlToProcess}: ${message}`); + res.statusCode = 500; + res.json({ message }); + } }); return router; diff --git a/plugins/adr/src/api/AdrClient.ts b/plugins/adr/src/api/AdrClient.ts index d79a327db3..da1adc04e4 100644 --- a/plugins/adr/src/api/AdrClient.ts +++ b/plugins/adr/src/api/AdrClient.ts @@ -51,7 +51,7 @@ export class AdrClient implements AdrApi { const data = await result.json(); if (!result.ok) { - throw new Error(data.error.message); + throw new Error(`${data.message}`); } return data; }