backend-common: review comments for readTree + switch archive return type to a stream

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<ReadTreeResponse> {
-    const parsed = new URL(repoUrl);
+  readTree(url: string, options?: ReadTreeOptions): Promise<ReadTreeResponse> {
+    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() {

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([

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<File[]> {
+  async files(): Promise<ReadTreeResponseFile[]> {
     this.onlyOnce();
 
-    const files: File[] = [];
+    const files = Array<ReadTreeResponseFile>();
     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<Buffer> {
+  async archive(): Promise<Readable> {
     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<Buffer>(async resolve => {
         await pipeline(
           tar.create({ cwd: tmpDir }, ['']),
           concatStream(resolve),
         );
       });
+      return Readable.from(data);
     } finally {
       await fs.remove(tmpDir);
     }

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;

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<Buffer>;
 };
@@ -57,7 +68,7 @@ export type ReadTreeResponseDirOptions = {
 };
 
 export type ReadTreeResponse = {
-  files(): Promise<File[]>;
-  archive(): Promise<Buffer>;
+  files(): Promise<ReadTreeResponseFile[]>;
+  archive(): Promise<NodeJS.ReadableStream>;
   dir(options?: ReadTreeResponseDirOptions): Promise<string>;
 };

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('');
           },
         };
       }