STRF-9019: fixed wrong reading from stream in archiveManager that was breaking stencil-download command

lib/archiveManager.js

@@ -6,67 +6,51 @@ const yauzl = require('yauzl');
 const fs = require('fs');
 const path = require('path');
 const { promisify } = require('util');
+const stream = require('stream');
 
-const { readFromStream } = require('./utils/asyncUtils');
+const pipeline = promisify(stream.pipeline);
 
 /**
  * @param {object}   options
  * @param {string}   options.zipPath
- * @param {string}   [options.fileToExtract] - filename to download only
+ * @param {string}   [options.fileToExtract] - filename to extract only
  * @param {string[]} [options.exclude] - paths of files and directories to exclude
  * @param {object}   [options.outputNames] - new names for some files. Format: { 'oldName1': 'newName1', ...}
  * @returns {Promise<void>}
  */
-async function extractZipFiles({ zipPath, fileToExtract, exclude, outputNames = {} }) {
+async function extractZipFiles({ zipPath, fileToExtract, exclude = [], outputNames = {} }) {
     let foundMatch = false;
 
     const zipFile = await promisify(yauzl.open)(zipPath, { lazyEntries: true });
 
     await new Promise((resolve, reject) => {
         zipFile.on('entry', async (entry) => {
             try {
-                const readableStream = await promisify(zipFile.openReadStream.bind(zipFile))(entry);
-
-                if (fileToExtract) {
-                    if (fileToExtract !== entry.fileName) {
-                        zipFile.readEntry();
-                        return;
-                    }
-                    foundMatch = true;
-                } else if (exclude && exclude.length) {
-                    // Do not process any file or directory within the exclude option
-                    for (const excludeItem of exclude) {
-                        if (entry.fileName.startsWith(excludeItem)) {
-                            zipFile.readEntry();
-                            return;
-                        }
-                    }
-                }
-
-                // If file is a directory, then move to next
-                if (/\/$/.test(entry.fileName)) {
+                foundMatch = fileToExtract === entry.fileName;
+                const isNotTheSearchedFile = fileToExtract && !foundMatch;
+                const isDirectory = /\/$/.test(entry.fileName);
+                const isExcluded =
+                    exclude.length &&
+                    // startsWith used to exclude files from the specified directories
+                    exclude.some((excludeItem) => entry.fileName.startsWith(excludeItem));
+
+                if (isDirectory || isNotTheSearchedFile || isExcluded) {
                     zipFile.readEntry();
                     return;
                 }
 
+                const outputFilePath = outputNames[entry.fileName] || entry.fileName;
+                const outputDir = path.parse(outputFilePath).dir;
                 // Create a directory if the parent directory does not exists
-                const parsedPath = path.parse(entry.fileName);
-
-                if (parsedPath.dir && !fs.existsSync(parsedPath.dir)) {
-                    fs.mkdirSync(parsedPath.dir, { recursive: true });
-                }
-
-                let fileData = await readFromStream(readableStream);
-                if (entry.fileName.endsWith('.json')) {
-                    // Make sure that the JSON file if valid
-                    fileData = JSON.stringify(JSON.parse(fileData), null, 2);
+                if (outputDir && !fs.existsSync(outputDir)) {
+                    await fs.promises.mkdir(outputDir, { recursive: true });
                 }
 
-                const outputFileName = outputNames[entry.fileName] || entry.fileName;
-                await promisify(fs.writeFile)(outputFileName, fileData, { flag: 'w+' });
+                const readableStream = await promisify(zipFile.openReadStream.bind(zipFile))(entry);
+                const writeStream = fs.createWriteStream(outputFilePath, { flag: 'w+' });
+                await pipeline(readableStream, writeStream);
 
-                // Close read if the requested file is found
-                if (fileToExtract) {
+                if (foundMatch) {
                     zipFile.close();
                     resolve();
                     return;

lib/archiveManager.spec.js

@@ -1,14 +1,17 @@
 const fs = require('fs');
 const path = require('path');
 const yauzl = require('yauzl');
+const tmp = require('tmp-promise');
 
+const MockWritableStream = require('../test/_mocks/MockWritableStream');
 const { extractZipFiles } = require('./archiveManager');
 
 describe('archiveManager', () => {
     describe('extractZipFiles', () => {
-        // We run tests for a real archive containing 2 files:
+        // We run tests for a real archive containing 3 files:
         // - config.json
         // - schema.json
+        // - meta/mobile_light.jpg
         const zipPath = path.join(
             process.cwd(),
             'test',
@@ -17,86 +20,115 @@ describe('archiveManager', () => {
             'valid',
             'mock-theme.zip',
         );
-        let fsWriteSub;
-        let yauzlOpenSpy;
 
-        beforeEach(() => {
-            jest.spyOn(console, 'log').mockImplementation(jest.fn());
-
-            fsWriteSub = jest
-                .spyOn(fs, 'writeFile')
-                .mockImplementation((name, config, options, callback) => {
-                    callback(false);
-                });
-        });
+        const stubFsWriteStream = () => {
+            return jest
+                .spyOn(fs, 'createWriteStream')
+                .mockImplementation(() => new MockWritableStream());
+        };
 
         afterEach(() => {
             jest.resetAllMocks();
             jest.restoreAllMocks();
         });
 
-        beforeEach(() => {
-            yauzlOpenSpy = jest.spyOn(yauzl, 'open');
-        });
-
         it('should call yauzl.open with the passed zipPath', async () => {
+            const yauzlOpenSpy = jest.spyOn(yauzl, 'open');
+            stubFsWriteStream();
+
             await extractZipFiles({ zipPath });
 
             expect(yauzlOpenSpy).toHaveBeenCalledTimes(1);
         });
 
         it('should save all the files from the zip archive taking into account options.outputNames', async () => {
-            const newConfigName = 'config2.json';
+            const fsCreateWriteStreamStub = stubFsWriteStream();
+            const newConfigName = 'new-path/config2.json';
             const outputNames = { 'config.json': newConfigName };
+
             await extractZipFiles({ zipPath, outputNames });
 
-            expect(fsWriteSub).toHaveBeenCalledTimes(2);
-            expect(fsWriteSub).toHaveBeenCalledWith(
+            expect(fsCreateWriteStreamStub).toHaveBeenCalledTimes(3);
+            expect(fsCreateWriteStreamStub).toHaveBeenCalledWith(
                 'schema.json',
-                expect.anything(),
                 expect.objectContaining({ flag: 'w+' }),
-                expect.any(Function),
             );
-            expect(fsWriteSub).toHaveBeenCalledWith(
+            expect(fsCreateWriteStreamStub).toHaveBeenCalledWith(
                 newConfigName,
-                expect.anything(),
                 expect.objectContaining({ flag: 'w+' }),
-                expect.any(Function),
             );
         });
 
         it('should not save files specified in options.exclude', async () => {
+            const fsCreateWriteStreamStub = stubFsWriteStream();
             const exclude = ['config.json'];
+
             await extractZipFiles({ zipPath, exclude });
 
-            expect(fsWriteSub).toHaveBeenCalledTimes(1);
-            expect(fsWriteSub).toHaveBeenCalledWith(
-                'schema.json',
-                expect.anything(),
+            expect(fsCreateWriteStreamStub).not.toHaveBeenCalledWith(
+                exclude[0],
                 expect.objectContaining({ flag: 'w+' }),
-                expect.any(Function),
             );
         });
 
         it('should save the file specified in options.fileToExtract only', async () => {
+            const fsCreateWriteStreamStub = stubFsWriteStream();
             const fileToExtract = 'config.json';
+
             await extractZipFiles({ zipPath, fileToExtract });
 
-            expect(fsWriteSub).toHaveBeenCalledTimes(1);
-            expect(fsWriteSub).toHaveBeenCalledWith(
+            expect(fsCreateWriteStreamStub).toHaveBeenCalledTimes(1);
+            expect(fsCreateWriteStreamStub).toHaveBeenCalledWith(
                 fileToExtract,
-                expect.anything(),
                 expect.objectContaining({ flag: 'w+' }),
-                expect.any(Function),
             );
         });
 
         it('should throw an error when the file with name options.fileToExtract was not found', async () => {
+            const fsCreateWriteStreamStub = stubFsWriteStream();
             const fileToExtract = 'I dont exist.txt';
 
             await expect(extractZipFiles({ zipPath, fileToExtract })).rejects.toThrow(/not found/);
 
-            expect(fsWriteSub).toHaveBeenCalledTimes(0);
+            expect(fsCreateWriteStreamStub).toHaveBeenCalledTimes(0);
+        });
+
+        it('should pass on errors happening during files extraction', async () => {
+            const error = new Error('something went wrong');
+            jest.spyOn(fs, 'createWriteStream').mockImplementation(() => {
+                throw error;
+            });
+
+            await expect(extractZipFiles({ zipPath })).rejects.toThrow(error);
+        });
+
+        it('should extract binary files properly', async () => {
+            const { path: tempThemePath, cleanup } = await tmp.dir({ unsafeCleanup: true });
+            const expectedImage = await fs.promises.readFile(
+                './test/_mocks/themes/valid/meta/mobile_light.jpg',
+            );
+            const fsCreateWriteStreamSpy = jest.spyOn(fs, 'createWriteStream');
+            const fileToExtract = 'meta/mobile_light.jpg';
+            const outputFilePath = path.join(tempThemePath, fileToExtract);
+            const outputNames = {
+                [fileToExtract]: outputFilePath,
+            };
+
+            await extractZipFiles({ zipPath, fileToExtract, outputNames });
+
+            const extractedImage = await fs.promises.readFile(outputFilePath);
+
+            expect(fsCreateWriteStreamSpy).toHaveBeenCalledTimes(1);
+            expect(fsCreateWriteStreamSpy).toHaveBeenCalledWith(
+                outputFilePath,
+                expect.objectContaining({ flag: 'w+' }),
+            );
+
+            // eslint-disable-next-line jest/prefer-to-have-length
+            expect(extractedImage.length).toStrictEqual(expectedImage.length);
+            expect(Buffer.compare(extractedImage, expectedImage)).toBe(0);
+
+            await cleanup();
         });
     });
 });

lib/utils/NetworkUtils.spec.js

@@ -1,4 +1,5 @@
-const { Writable, PassThrough } = require('stream');
+const { PassThrough } = require('stream');
+const MockWritableStream = require('../../test/_mocks/MockWritableStream');
 const NetworkUtils = require('./NetworkUtils');
 
 describe('NetworkUtils', () => {
@@ -175,22 +176,6 @@ describe('NetworkUtils', () => {
     });
 
     describe('fetchFile', () => {
-        class MockWritableStream extends Writable {
-            constructor() {
-                super();
-                this.buffer = '';
-            }
-
-            _write(chunk, _, next) {
-                this.buffer += chunk;
-                next();
-            }
-
-            reset() {
-                this.buffer = '';
-            }
-        }
-
         it('should pipe the response from the request library to fs', async () => {
             const mockWritable = new MockWritableStream();
             const mockReadable = new PassThrough();

lib/utils/asyncUtils.js

@@ -3,19 +3,17 @@
  */
 
 /**
+ * WARNING! Can be used with text content only. Binary data (e.g. images) will get broken!
+ *
  * @param {ReadableStream} stream
  * @returns {Promise<string>}
  */
-function readFromStream(stream) {
-    return new Promise((resolve, reject) => {
-        let data = '';
-
-        stream.on('data', (chunk) => {
-            data += chunk;
-        });
-        stream.on('end', () => resolve(data));
-        stream.on('error', (error) => reject(error));
-    });
+async function readFromStream(stream) {
+    let result = '';
+    for await (const chunk of stream) {
+        result += chunk;
+    }
+    return result;
 }
 
 module.exports = {

test/_mocks/MockWritableStream.js

@@ -0,0 +1,19 @@
+const { Writable } = require('stream');
+
+class MockWritableStream extends Writable {
+    constructor() {
+        super();
+        this.buffer = '';
+    }
+
+    _write(chunk, _, next) {
+        this.buffer += chunk;
+        next();
+    }
+
+    reset() {
+        this.buffer = '';
+    }
+}
+
+module.exports = MockWritableStream;