From 3ecd59147541c185f0820b99e712cd085b439b48 Mon Sep 17 00:00:00 2001 From: Djamaile Rahamat Date: Fri, 18 Jun 2021 15:24:17 +0200 Subject: [PATCH 1/8] fix: ignore images and files that are bigger than/equal 200k Signed-off-by: Djamaile Rahamat --- .../src/reading/tree/TarArchiveResponse.ts | 5 +++++ .../src/reading/tree/ZipArchiveResponse.ts | 5 +++++ .../src/lib/TodoReader/TodoScmReader.ts | 20 +++++++++++++++++-- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/packages/backend-common/src/reading/tree/TarArchiveResponse.ts b/packages/backend-common/src/reading/tree/TarArchiveResponse.ts index 173f6a886a..adcf420fc0 100644 --- a/packages/backend-common/src/reading/tree/TarArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/TarArchiveResponse.ts @@ -98,6 +98,11 @@ export class TarArchiveResponse implements ReadTreeResponse { } } + if (entry.size && entry.size >= 20000) { + entry.resume(); + return; + } + const content = new Promise(async resolve => { await pipeline(entry, concatStream(resolve)); }); diff --git a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts index 45c6880a55..62219ec27d 100644 --- a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts @@ -68,6 +68,11 @@ export class ZipArchiveResponse implements ReadTreeResponse { private shouldBeIncluded(entry: Entry): boolean { const strippedPath = stripFirstDirectoryFromPath(entry.path); + const size = entry.vars.compressedSize; + + if (size >= 20000) { + return false; + } if (this.subPath) { if (!strippedPath.startsWith(this.subPath)) { diff --git a/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts b/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts index 256279596f..1e9cb68bd9 100644 --- a/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts +++ b/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts @@ -27,6 +27,7 @@ import { } from './types'; import { Config } from '@backstage/config'; import { createTodoParser } from './createTodoParser'; +import path from 'path'; type Options = { logger: Logger; @@ -80,10 +81,25 @@ export class TodoScmReader implements TodoReader { { url }: ReadTodosOptions, etag?: string, ): Promise { + const shouldNotInclude = [ + '.png', + '.svg', + '.jpg', + '.jpeg', + '.gif', + '.raw', + '.lock', + '.ico', + ]; const tree = await this.reader.readTree(url, { etag, - filter(path) { - return !path.startsWith('.') && !path.includes('/.'); + filter(filePath) { + const extname = path.extname(filePath); + return ( + !filePath.startsWith('.') && + !filePath.includes('/.') && + !shouldNotInclude.includes(extname) + ); }, }); From 0b34dc7a328fea9c00cc1d18d4ceea2a29ed9a19 Mon Sep 17 00:00:00 2001 From: Djamaile Rahamat Date: Fri, 18 Jun 2021 15:29:13 +0200 Subject: [PATCH 2/8] chore: add changeset Signed-off-by: Djamaile Rahamat --- .changeset/mean-spiders-run.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/mean-spiders-run.md diff --git a/.changeset/mean-spiders-run.md b/.changeset/mean-spiders-run.md new file mode 100644 index 0000000000..10a1208971 --- /dev/null +++ b/.changeset/mean-spiders-run.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-todo-backend': minor +--- + +images will be ignored and files bigger than 200Kb will also be ignored so that the todo plugin doesn't stay hanging From 62eb025cab2486e70f2c4f8426caa2533ef3d40a Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 16 Aug 2021 13:30:43 +0200 Subject: [PATCH 3/8] changesets: tweak todo-backend changeset Signed-off-by: Patrik Oldsberg --- .changeset/mean-spiders-run.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.changeset/mean-spiders-run.md b/.changeset/mean-spiders-run.md index 10a1208971..b9d8e5efc4 100644 --- a/.changeset/mean-spiders-run.md +++ b/.changeset/mean-spiders-run.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-todo-backend': minor +'@backstage/plugin-todo-backend': patch --- -images will be ignored and files bigger than 200Kb will also be ignored so that the todo plugin doesn't stay hanging +Ignore images and files that are larger than 200KB. From 7b438e6f3579872cb316ccc3aade5e3dff6149b4 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 16 Aug 2021 13:31:52 +0200 Subject: [PATCH 4/8] backend-common: remove hardcoded size check and replace with filter param Signed-off-by: Patrik Oldsberg --- .../src/reading/tree/TarArchiveResponse.ts | 13 ++++--------- .../src/reading/tree/ZipArchiveResponse.ts | 13 ++++++------- packages/backend-common/src/reading/types.ts | 4 ++-- 3 files changed, 12 insertions(+), 18 deletions(-) diff --git a/packages/backend-common/src/reading/tree/TarArchiveResponse.ts b/packages/backend-common/src/reading/tree/TarArchiveResponse.ts index adcf420fc0..f7ab7f5349 100644 --- a/packages/backend-common/src/reading/tree/TarArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/TarArchiveResponse.ts @@ -43,7 +43,7 @@ export class TarArchiveResponse implements ReadTreeResponse { private readonly subPath: string, private readonly workDir: string, public readonly etag: string, - private readonly filter?: (path: string) => boolean, + private readonly filter?: (path: string, info: { size: number }) => boolean, ) { if (subPath) { if (!subPath.endsWith('/')) { @@ -92,17 +92,12 @@ export class TarArchiveResponse implements ReadTreeResponse { const path = relativePath.slice(this.subPath.length); if (this.filter) { - if (!this.filter(path)) { + if (!this.filter(path, { size: entry.remain })) { entry.resume(); return; } } - if (entry.size && entry.size >= 20000) { - entry.resume(); - return; - } - const content = new Promise(async resolve => { await pipeline(entry, concatStream(resolve)); }); @@ -160,7 +155,7 @@ export class TarArchiveResponse implements ReadTreeResponse { tar.extract({ strip, cwd: dir, - filter: path => { + filter: (path, stat) => { // File path relative to the root extracted directory. Will remove the // top level dir name from the path since its name is hard to predetermine. const relativePath = stripFirstDirectoryFromPath(path); @@ -169,7 +164,7 @@ export class TarArchiveResponse implements ReadTreeResponse { } if (this.filter) { const innerPath = path.split('/').slice(strip).join('/'); - return this.filter(innerPath); + return this.filter(innerPath, { size: stat.size }); } return true; }, diff --git a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts index 62219ec27d..5c12a383ee 100644 --- a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts @@ -37,7 +37,7 @@ export class ZipArchiveResponse implements ReadTreeResponse { private readonly subPath: string, private readonly workDir: string, public readonly etag: string, - private readonly filter?: (path: string) => boolean, + private readonly filter?: (path: string, info: { size: number }) => boolean, ) { if (subPath) { if (!subPath.endsWith('/')) { @@ -68,11 +68,6 @@ export class ZipArchiveResponse implements ReadTreeResponse { private shouldBeIncluded(entry: Entry): boolean { const strippedPath = stripFirstDirectoryFromPath(entry.path); - const size = entry.vars.compressedSize; - - if (size >= 20000) { - return false; - } if (this.subPath) { if (!strippedPath.startsWith(this.subPath)) { @@ -80,7 +75,11 @@ export class ZipArchiveResponse implements ReadTreeResponse { } } if (this.filter) { - return this.filter(this.getInnerPath(entry.path)); + return this.filter(this.getInnerPath(entry.path), { + size: + (entry.vars as { uncompressedSize?: number }).uncompressedSize ?? + entry.vars.compressedSize, + }); } return true; } diff --git a/packages/backend-common/src/reading/types.ts b/packages/backend-common/src/reading/types.ts index 8efc833ead..93f287d4fe 100644 --- a/packages/backend-common/src/reading/types.ts +++ b/packages/backend-common/src/reading/types.ts @@ -104,7 +104,7 @@ export type ReadTreeOptions = { * * If no filter is provided all files are extracted. */ - filter?(path: string): boolean; + filter?(path: string, info?: { size: number }): boolean; /** * An etag can be provided to check whether readTree's response has changed from a previous execution. @@ -164,7 +164,7 @@ export type FromArchiveOptions = { // etag of the blob etag: string; // Filter passed on from the ReadTreeOptions - filter?: (path: string) => boolean; + filter?: (path: string, info?: { size: number }) => boolean; }; export interface ReadTreeResponseFactory { From 89966c0c37b01e2fe08f95bea2cf818a84d766f5 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 16 Aug 2021 13:32:39 +0200 Subject: [PATCH 5/8] todo-backend: filter out large or binary files + deduplicate in-flight requests Signed-off-by: Patrik Oldsberg --- .../src/lib/TodoReader/TodoScmReader.ts | 38 ++++++++++++------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts b/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts index 1e9cb68bd9..2bd0f0b038 100644 --- a/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts +++ b/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts @@ -29,6 +29,18 @@ import { Config } from '@backstage/config'; import { createTodoParser } from './createTodoParser'; import path from 'path'; +const excludedExtensions = [ + '.png', + '.svg', + '.jpg', + '.jpeg', + '.gif', + '.raw', + '.lock', + '.ico', +]; +const MAX_FILE_SIZE = 200000; + type Options = { logger: Logger; reader: UrlReader; @@ -48,6 +60,7 @@ export class TodoScmReader implements TodoReader { private readonly integrations: ScmIntegrations; private readonly cache = new Map(); + private readonly inFlightReads = new Map>(); static fromConfig(config: Config, options: Omit) { return new TodoScmReader({ @@ -66,7 +79,13 @@ export class TodoScmReader implements TodoReader { async readTodos({ url }: ReadTodosOptions): Promise { const cacheItem = this.cache.get(url); try { - const newCacheItem = await this.doReadTodos({ url }, cacheItem?.etag); + const inFlightRead = this.inFlightReads.get(url); + if (inFlightRead) { + return (await inFlightRead).result; + } + const newRead = this.doReadTodos({ url }, cacheItem?.etag); + this.inFlightReads.set(url, newRead); + const newCacheItem = await newRead; this.cache.set(url, newCacheItem); return newCacheItem.result; } catch (error) { @@ -81,24 +100,17 @@ export class TodoScmReader implements TodoReader { { url }: ReadTodosOptions, etag?: string, ): Promise { - const shouldNotInclude = [ - '.png', - '.svg', - '.jpg', - '.jpeg', - '.gif', - '.raw', - '.lock', - '.ico', - ]; const tree = await this.reader.readTree(url, { etag, - filter(filePath) { + filter(filePath, info) { const extname = path.extname(filePath); + if (info && info.size > MAX_FILE_SIZE) { + return false; + } return ( !filePath.startsWith('.') && !filePath.includes('/.') && - !shouldNotInclude.includes(extname) + !excludedExtensions.includes(extname) ); }, }); From 8543d989068241e739894f6b81b2220e1abe8a71 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 16 Aug 2021 13:34:34 +0200 Subject: [PATCH 6/8] changesets: add changeset for readTree filter addition Signed-off-by: Patrik Oldsberg --- .changeset/fair-otters-applaud.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fair-otters-applaud.md diff --git a/.changeset/fair-otters-applaud.md b/.changeset/fair-otters-applaud.md new file mode 100644 index 0000000000..8e35c6fe6d --- /dev/null +++ b/.changeset/fair-otters-applaud.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Add an optional `info` parameter to the `readTree` filter option with a `size` property. From c8fd2fc190a3b93b441950aa1d394c3be2a572fb Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 17 Aug 2021 11:43:04 +0200 Subject: [PATCH 7/8] backend-common: update API report Signed-off-by: Patrik Oldsberg --- packages/backend-common/api-report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index 2652dad122..736899d1ed 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -593,7 +593,7 @@ export function useHotMemoize(_module: NodeModule, valueFactory: () => T): T; // src/cache/types.d.ts:34:5 - (ae-forgotten-export) The symbol "ClientOptions" needs to be exported by the entry point index.d.ts // src/middleware/errorHandler.d.ts:17:26 - (tsdoc-malformed-html-name) Invalid HTML element: A space is not allowed here // src/reading/AzureUrlReader.d.ts:9:9 - (ae-forgotten-export) The symbol "ReadTreeResponseFactory" needs to be exported by the entry point index.d.ts -// src/reading/types.d.ts:106:5 - (ae-forgotten-export) The symbol "ReadTreeResponseDirOptions" needs to be exported by the entry point index.d.ts +// src/reading/types.d.ts:108:5 - (ae-forgotten-export) The symbol "ReadTreeResponseDirOptions" needs to be exported by the entry point index.d.ts // src/service/types.d.ts:12:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen // src/service/types.d.ts:22:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen // src/service/types.d.ts:30:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen From 8f2c880b80441d9cc9554dee15a7e4d1dc458933 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 17 Aug 2021 13:55:13 +0200 Subject: [PATCH 8/8] todo-backend: fix TodoScmReader caching logic Signed-off-by: Patrik Oldsberg --- .../src/lib/TodoReader/TodoScmReader.test.ts | 8 +++++- .../src/lib/TodoReader/TodoScmReader.ts | 26 +++++++++++-------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.test.ts b/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.test.ts index a3fc607404..9675a285e0 100644 --- a/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.test.ts +++ b/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.test.ts @@ -74,7 +74,13 @@ describe('TodoScmReader', () => { ], }; - await expect(todoReader.readTodos({ url })).resolves.toEqual(expected); + // These two reads should only result in a single call to readTree + await expect( + Promise.all([ + todoReader.readTodos({ url }), + todoReader.readTodos({ url }), + ]), + ).resolves.toEqual([expected, expected]); expect(reader.readTree).toHaveBeenCalledTimes(1); expect(reader.readTree).toHaveBeenCalledWith( diff --git a/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts b/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts index 2bd0f0b038..8442983637 100644 --- a/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts +++ b/plugins/todo-backend/src/lib/TodoReader/TodoScmReader.ts @@ -77,22 +77,26 @@ export class TodoScmReader implements TodoReader { } async readTodos({ url }: ReadTodosOptions): Promise { + const inFlightRead = this.inFlightReads.get(url); + if (inFlightRead) { + return inFlightRead.then(read => read.result); + } + const cacheItem = this.cache.get(url); - try { - const inFlightRead = this.inFlightReads.get(url); - if (inFlightRead) { - return (await inFlightRead).result; + const newRead = this.doReadTodos({ url }, cacheItem?.etag).catch(error => { + if (cacheItem && error.name === 'NotModifiedError') { + return cacheItem; } - const newRead = this.doReadTodos({ url }, cacheItem?.etag); - this.inFlightReads.set(url, newRead); + throw error; + }); + + this.inFlightReads.set(url, newRead); + try { const newCacheItem = await newRead; this.cache.set(url, newCacheItem); return newCacheItem.result; - } catch (error) { - if (cacheItem && error.name === 'NotModifiedError') { - return cacheItem.result; - } - throw error; + } finally { + this.inFlightReads.delete(url); } }