From bd1cef1847bb2f2c75d7eda147a68a868f06b31e Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 13 Nov 2020 00:43:20 +0100 Subject: [PATCH] backend-common: review comments for readTree + switch archive return type to a stream --- .../src/reading/UrlReaderPredicateMux.ts | 13 ++++------ .../src/reading/tree/ArchiveResponse.test.ts | 3 +-- .../src/reading/tree/ArchiveResponse.ts | 26 +++++++++---------- .../reading/tree/ReadTreeResponseFactory.ts | 2 +- packages/backend-common/src/reading/types.ts | 19 +++++++++++--- plugins/techdocs-backend/src/helpers.test.ts | 3 ++- 6 files changed, 37 insertions(+), 29 deletions(-) diff --git a/packages/backend-common/src/reading/UrlReaderPredicateMux.ts b/packages/backend-common/src/reading/UrlReaderPredicateMux.ts index 9a81fdc9ab..465c125fda 100644 --- a/packages/backend-common/src/reading/UrlReaderPredicateMux.ts +++ b/packages/backend-common/src/reading/UrlReaderPredicateMux.ts @@ -58,23 +58,20 @@ export class UrlReaderPredicateMux implements UrlReader { throw new Error(`No reader found that could handle '${url}'`); } - readTree( - repoUrl: string, - options?: ReadTreeOptions, - ): Promise { - const parsed = new URL(repoUrl); + readTree(url: string, options?: ReadTreeOptions): Promise { + const parsed = new URL(url); for (const { predicate, reader } of this.readers) { if (predicate(parsed)) { - return reader.readTree(repoUrl, options); + return reader.readTree(url, options); } } if (this.fallback) { - return this.fallback.readTree(repoUrl, options); + return this.fallback.readTree(url, options); } - throw new Error(`No reader found that could handle '${repoUrl}'`); + throw new Error(`No reader found that could handle '${url}'`); } toString() { diff --git a/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts b/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts index 6b8f29de8c..fd351863b2 100644 --- a/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts +++ b/packages/backend-common/src/reading/tree/ArchiveResponse.test.ts @@ -16,7 +16,6 @@ import fs from 'fs-extra'; import mockFs from 'mock-fs'; -import { Readable } from 'stream'; import { resolve as resolvePath } from 'path'; import { ArchiveResponse } from './ArchiveResponse'; @@ -87,7 +86,7 @@ describe('ArchiveResponse', () => { 'Response has already been read', ); - const res2 = new ArchiveResponse(Readable.from(buffer), '', '/tmp'); + const res2 = new ArchiveResponse(buffer, '', '/tmp'); const files = await res2.files(); expect(files).toEqual([ diff --git a/packages/backend-common/src/reading/tree/ArchiveResponse.ts b/packages/backend-common/src/reading/tree/ArchiveResponse.ts index 0e06ff3804..e16be63127 100644 --- a/packages/backend-common/src/reading/tree/ArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/ArchiveResponse.ts @@ -14,14 +14,17 @@ * limitations under the License. */ -import os from 'os'; import tar, { Parse, ParseStream, ReadEntry } from 'tar'; import path from 'path'; import fs from 'fs-extra'; import { Readable, pipeline as pipelineCb } from 'stream'; import { promisify } from 'util'; import concatStream from 'concat-stream'; -import { ReadTreeResponse, File, ReadTreeResponseDirOptions } from '../types'; +import { + ReadTreeResponse, + ReadTreeResponseFile, + ReadTreeResponseDirOptions, +} from '../types'; // Tar types for `Parse` is not a proper constructor, but it should be const TarParseStream = (Parse as unknown) as { new (): ParseStream }; @@ -37,7 +40,7 @@ export class ArchiveResponse implements ReadTreeResponse { constructor( private readonly stream: Readable, private readonly subPath: string, - private readonly workDir: string = os.tmpdir(), + private readonly workDir: string, private readonly filter?: (path: string) => boolean, ) { if (subPath) { @@ -60,10 +63,10 @@ export class ArchiveResponse implements ReadTreeResponse { this.read = true; } - async files(): Promise { + async files(): Promise { this.onlyOnce(); - const files: File[] = []; + const files = Array(); const parser = new TarParseStream(); parser.on('entry', (entry: ReadEntry & Readable) => { @@ -79,9 +82,7 @@ export class ArchiveResponse implements ReadTreeResponse { } } - const path = this.subPath - ? entry.path.replace(this.subPath, '') - : entry.path; + const path = entry.path.slice(this.subPath.length); if (this.filter) { if (!this.filter(path)) { entry.resume(); @@ -103,13 +104,11 @@ export class ArchiveResponse implements ReadTreeResponse { return files; } - async archive(): Promise { + async archive(): Promise { if (!this.subPath) { this.onlyOnce(); - return new Promise(resolve => - pipeline(this.stream, concatStream(resolve)), - ); + return this.stream; } // TODO(Rugvip): method for repacking a tar with a subpath is to simply extract into a @@ -117,12 +116,13 @@ export class ArchiveResponse implements ReadTreeResponse { const tmpDir = await this.dir(); try { - return await new Promise(async resolve => { + const data = await new Promise(async resolve => { await pipeline( tar.create({ cwd: tmpDir }, ['']), concatStream(resolve), ); }); + return Readable.from(data); } finally { await fs.remove(tmpDir); } diff --git a/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts b/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts index 5044cdabdc..a134d185f3 100644 --- a/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts +++ b/packages/backend-common/src/reading/tree/ReadTreeResponseFactory.ts @@ -23,7 +23,7 @@ import { ArchiveResponse } from './ArchiveResponse'; type FromArchiveOptions = { // A binary stream of a tar archive. stream: Readable; - // If set, root of the tree will be set to the given path. Should not have a trailing `/`. + // If set, the root of the tree will be set to the given directory path. path?: string; // Filter passed on from the ReadTreeOptions filter?: (path: string) => boolean; diff --git a/packages/backend-common/src/reading/types.ts b/packages/backend-common/src/reading/types.ts index 101502fed7..f9dca3e1d5 100644 --- a/packages/backend-common/src/reading/types.ts +++ b/packages/backend-common/src/reading/types.ts @@ -19,7 +19,18 @@ import { Config } from '@backstage/config'; import { ReadTreeResponseFactory } from './tree'; export type ReadTreeOptions = { - /** A filter that can be used to select which files should be extracted. By default all files are extracted */ + /** + * A filter that can be used to select which files should be included. + * + * The path passed to the filter function is the relative path from the URL + * that the file tree is fetched from, without any leading '/'. + * + * For example, given the URL https://github.com/my/repo/tree/master/my-dir, a file + * at https://github.com/my/repo/blob/master/my-dir/my-subdir/my-file.txt will + * be represented as my-subdir/my-file.txt + * + * If no filter is provided all files are extracted. + */ filter?(path: string): boolean; }; @@ -46,7 +57,7 @@ export type ReaderFactory = (options: { treeResponseFactory: ReadTreeResponseFactory; }) => UrlReaderPredicateTuple[]; -export type File = { +export type ReadTreeResponseFile = { path: string; content(): Promise; }; @@ -57,7 +68,7 @@ export type ReadTreeResponseDirOptions = { }; export type ReadTreeResponse = { - files(): Promise; - archive(): Promise; + files(): Promise; + archive(): Promise; dir(options?: ReadTreeResponseDirOptions): Promise; }; diff --git a/plugins/techdocs-backend/src/helpers.test.ts b/plugins/techdocs-backend/src/helpers.test.ts index 5e98ab51c8..10518df74f 100644 --- a/plugins/techdocs-backend/src/helpers.test.ts +++ b/plugins/techdocs-backend/src/helpers.test.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { Readable } from 'stream'; import { getDocFilesFromRepository } from './helpers'; import { UrlReader, ReadTreeResponse } from '@backstage/backend-common'; import { Entity } from '@backstage/catalog-model'; @@ -34,7 +35,7 @@ describe('getDocFilesFromRepository', () => { return []; }, archive: async () => { - return Buffer.from(''); + return Readable.from(''); }, }; }