From 22e9262868b1bec6187645f4b50c62e802ed9bdd Mon Sep 17 00:00:00 2001
From: blam <ben@blam.sh>
Date: Thu, 21 Jul 2022 16:20:19 +0200
Subject: [PATCH] chore: code-review comments

Signed-off-by: blam <ben@blam.sh>
---
 .../src/reading/tree/ZipArchiveResponse.ts    | 27 +++++++++++++------
 .../backend-common/src/reading/tree/util.ts   | 22 +++++++++------
 2 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts
index 15bc0d0258..8126d1fcdc 100644
--- a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts
+++ b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts
@@ -80,7 +80,9 @@ export class ZipArchiveResponse implements ReadTreeResponse {
     return true;
   }
 
-  private async streamToTemporaryFile(stream: Readable): Promise<string> {
+  private async streamToTemporaryFile(
+    stream: Readable,
+  ): Promise<{ fileName: string; cleanup: () => Promise<void> }> {
     const tmpDir = await fs.mkdtemp(
       platformPath.join(this.workDir, 'backstage-tmp'),
     );
@@ -90,7 +92,9 @@ export class ZipArchiveResponse implements ReadTreeResponse {
 
     return new Promise((resolve, reject) => {
       writeStream.on('error', reject);
-      writeStream.on('finish', () => resolve(tmpFile));
+      writeStream.on('finish', () =>
+        resolve({ fileName: tmpFile, cleanup: () => fs.remove(tmpFile) }),
+      );
       stream.pipe(writeStream);
     });
   }
@@ -123,6 +127,7 @@ export class ZipArchiveResponse implements ReadTreeResponse {
           zipfile.readEntry();
         });
         zipfile.once('end', () => resolve());
+        zipfile.on('error', e => reject(e));
         zipfile.readEntry();
       });
     });
@@ -131,15 +136,17 @@ export class ZipArchiveResponse implements ReadTreeResponse {
   async files(): Promise<ReadTreeResponseFile[]> {
     this.onlyOnce();
     const files = Array<ReadTreeResponseFile>();
-    const tmpFile = await this.streamToTemporaryFile(this.stream);
+    const temporary = await this.streamToTemporaryFile(this.stream);
 
-    await this.forEveryZipEntry(tmpFile, async (entry, content) => {
+    await this.forEveryZipEntry(temporary.fileName, async (entry, content) => {
       files.push({
         path: this.getInnerPath(entry.fileName),
         content: async () => await streamToBuffer(content),
       });
     });
 
+    temporary.cleanup();
+
     return files;
   }
 
@@ -151,9 +158,9 @@ export class ZipArchiveResponse implements ReadTreeResponse {
     }
 
     const archive = archiver('zip');
-    const tmpFile = await this.streamToTemporaryFile(this.stream);
+    const temporary = await this.streamToTemporaryFile(this.stream);
 
-    await this.forEveryZipEntry(tmpFile, async (entry, content) => {
+    await this.forEveryZipEntry(temporary.fileName, async (entry, content) => {
       archive.append(await streamToBuffer(content), {
         name: this.getInnerPath(entry.fileName),
       });
@@ -161,6 +168,8 @@ export class ZipArchiveResponse implements ReadTreeResponse {
 
     archive.finalize();
 
+    temporary.cleanup();
+
     return archive;
   }
 
@@ -170,9 +179,9 @@ export class ZipArchiveResponse implements ReadTreeResponse {
       options?.targetDir ??
       (await fs.mkdtemp(platformPath.join(this.workDir, 'backstage-')));
 
-    const tmpFile = await this.streamToTemporaryFile(this.stream);
+    const temporary = await this.streamToTemporaryFile(this.stream);
 
-    await this.forEveryZipEntry(tmpFile, async (entry, content) => {
+    await this.forEveryZipEntry(temporary.fileName, async (entry, content) => {
       const entryPath = this.getInnerPath(entry.fileName);
       const dirname = platformPath.dirname(entryPath);
 
@@ -187,6 +196,8 @@ export class ZipArchiveResponse implements ReadTreeResponse {
       });
     });
 
+    temporary.cleanup();
+
     return dir;
   }
 }
diff --git a/packages/backend-common/src/reading/tree/util.ts b/packages/backend-common/src/reading/tree/util.ts
index bfe02ac6c8..63192102f4 100644
--- a/packages/backend-common/src/reading/tree/util.ts
+++ b/packages/backend-common/src/reading/tree/util.ts
@@ -14,22 +14,28 @@
  * limitations under the License.
  */
 
+import { Readable, pipeline as pipelineCb } from 'stream';
+import { promisify } from 'util';
+import concatStream from 'concat-stream';
+
+const pipeline = promisify(pipelineCb);
+
 // Matches a directory name + one `/` at the start of any string,
 // containing any character except `/` one or more times, and ending with a `/`
 // e.g. Will match `dirA/` in `dirA/dirB/file.ext`
 const directoryNameRegex = /^[^\/]+\//;
-import { Readable } from 'stream';
 // Removes the first segment of a forward-slash-separated path
 export function stripFirstDirectoryFromPath(path: string): string {
   return path.replace(directoryNameRegex, '');
 }
 
-// Concats the data into a buffer.
-export const streamToBuffer = async (stream: Readable): Promise<Buffer> => {
-  const buffers: Buffer[] = [];
-  return new Promise((resolve, reject) => {
-    stream.on('data', (data: Buffer) => buffers.push(data));
-    stream.on('error', reject);
-    stream.on('end', () => resolve(Buffer.concat(buffers)));
+// Collect the stream into a buffer and return
+export const streamToBuffer = (stream: Readable): Promise<Buffer> => {
+  return new Promise(async (resolve, reject) => {
+    try {
+      await pipeline(stream, concatStream(resolve));
+    } catch (ex) {
+      reject(ex);
+    }
   });
 };