fix: (STRF-9019) wrong reading from stream breaks stencil-download co…

…mmand

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;