From 9f62429dad234167dc6f0969b40c7942bab83aee Mon Sep 17 00:00:00 2001 From: hochoy Date: Tue, 21 May 2024 08:17:47 -0700 Subject: [PATCH] fix: allow files in directories to be downloaded onto local machine (#2199) * fix: allow files in nested folders to be downloaded onto local machine * fix: revert removal of options.prefix check * test: file.download should create directory paths recursively * fix: move the filtering of directory objects from TransferManager.downloadManyFiles to File.download * fix: handle windows path * chore: remove temporary comments * test: add scenarios where destination is set in Transfer Manager * chore: remove console.warn * fix: merge with main * fix: added transfer manager tests for prefix * fix: address PR comments * fix: paths for windows ci * fix: migrate mkdirsync to await fs/promises/mkdir * fix: var name --------- Co-authored-by: Vishwaraj Anand --- src/file.ts | 3 +- src/transfer-manager.ts | 20 +++++++++-- test/file.ts | 30 +++++++++++++++++ test/transfer-manager.ts | 73 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 4 deletions(-) diff --git a/src/file.ts b/src/file.ts index 14024cb2b..32f1f62b7 100644 --- a/src/file.ts +++ b/src/file.ts @@ -2288,12 +2288,13 @@ class File extends ServiceObject { const fileStream = this.createReadStream(options); let receivedData = false; + if (destination) { fileStream .on('error', callback) .once('data', data => { - // We know that the file exists the server - now we can truncate/write to a file receivedData = true; + // We know that the file exists the server - now we can truncate/write to a file const writable = fs.createWriteStream(destination); writable.write(data); fileStream diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index 8b361b688..d0fe21f75 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -526,7 +526,9 @@ export class TransferManager { * * @param {array | string} [filesOrFolder] An array of file name strings or file objects to be downloaded. If * a string is provided this will be treated as a GCS prefix and all files with that prefix will be downloaded. - * @param {DownloadManyFilesOptions} [options] Configuration options. + * @param {DownloadManyFilesOptions} [options] Configuration options. Setting options.prefix or options.stripPrefix + * or options.passthroughOptions.destination will cause the downloaded files to be written to the file system + * instead of being returned as a buffer. * @returns {Promise} * * @example @@ -587,7 +589,7 @@ export class TransferManager { [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, }; - if (options.prefix) { + if (options.prefix || passThroughOptionsCopy.destination) { passThroughOptionsCopy.destination = path.join( options.prefix || '', passThroughOptionsCopy.destination || '', @@ -598,7 +600,19 @@ export class TransferManager { passThroughOptionsCopy.destination = file.name.replace(regex, ''); } - promises.push(limit(() => file.download(passThroughOptionsCopy))); + promises.push( + limit(async () => { + const destination = passThroughOptionsCopy.destination; + if (destination && destination.endsWith(path.sep)) { + await fsp.mkdir(destination, {recursive: true}); + return Promise.resolve([ + Buffer.alloc(0), + ]) as Promise; + } + + return file.download(passThroughOptionsCopy); + }) + ); } return Promise.all(promises); diff --git a/test/file.ts b/test/file.ts index bd60273f4..5369083af 100644 --- a/test/file.ts +++ b/test/file.ts @@ -28,6 +28,7 @@ import assert from 'assert'; import * as crypto from 'crypto'; import duplexify from 'duplexify'; import * as fs from 'fs'; +import * as path from 'path'; import proxyquire from 'proxyquire'; import * as resumableUpload from '../src/resumable-upload.js'; import * as sinon from 'sinon'; @@ -2571,6 +2572,12 @@ describe('File', () => { }); describe('with destination', () => { + const sandbox = sinon.createSandbox(); + + afterEach(() => { + sandbox.restore(); + }); + it('should write the file to a destination if provided', done => { tmp.setGracefulCleanup(); tmp.file((err, tmpFilePath) => { @@ -2694,6 +2701,29 @@ describe('File', () => { }); }); }); + + it('should fail if provided destination directory does not exist', done => { + tmp.setGracefulCleanup(); + tmp.dir(async (err, tmpDirPath) => { + assert.ifError(err); + + const fileContents = 'nested-abcdefghijklmnopqrstuvwxyz'; + + Object.assign(fileReadStream, { + _read(this: Readable) { + this.push(fileContents); + this.push(null); + }, + }); + + const nestedPath = path.join(tmpDirPath, 'a', 'b', 'c', 'file.txt'); + + file.download({destination: nestedPath}, (err: Error) => { + assert.ok(err); + done(); + }); + }); + }); }); }); diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 4c5d3b50f..741e0a91c 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -19,6 +19,7 @@ import { Bucket, File, CRC32C, + DownloadCallback, DownloadOptions, IdempotencyStrategy, MultiPartHelperGenerator, @@ -233,6 +234,78 @@ describe('Transfer Manager', () => { await transferManager.downloadManyFiles([file]); }); + + it('sets the destination correctly when provided a passthroughOptions.destination', async () => { + const passthroughOptions = { + destination: 'test-destination', + }; + const filename = 'first.txt'; + const expectedDestination = path.normalize( + `${passthroughOptions.destination}/${filename}` + ); + const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { + if (typeof optionsOrCb === 'function') { + optionsOrCb(null, Buffer.alloc(0)); + } else if (optionsOrCb) { + assert.strictEqual(optionsOrCb.destination, expectedDestination); + } + return Promise.resolve([Buffer.alloc(0)]) as Promise; + }; + + const file = new File(bucket, filename); + file.download = download; + await transferManager.downloadManyFiles([file], {passthroughOptions}); + }); + + it('does not set the destination when prefix, strip prefix and passthroughOptions.destination are not provided', async () => { + const options = {}; + const filename = 'first.txt'; + const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { + if (typeof optionsOrCb === 'function') { + optionsOrCb(null, Buffer.alloc(0)); + } else if (optionsOrCb) { + assert.strictEqual(optionsOrCb.destination, undefined); + } + return Promise.resolve([Buffer.alloc(0)]) as Promise; + }; + + const file = new File(bucket, filename); + file.download = download; + await transferManager.downloadManyFiles([file], options); + }); + + it('should recursively create directory and write file contents if destination path is nested', async () => { + const prefix = 'text-prefix'; + const folder = 'nestedFolder/'; + const file = 'first.txt'; + const filesOrFolder = [folder, path.join(folder, file)]; + const expectedFilePath = path.join(prefix, folder, file); + const expectedDir = path.join(prefix, folder); + const mkdirSpy = sandbox.spy(fsp, 'mkdir'); + const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { + if (typeof optionsOrCb === 'function') { + optionsOrCb(null, Buffer.alloc(0)); + } else if (optionsOrCb) { + assert.strictEqual(optionsOrCb.destination, expectedFilePath); + } + return Promise.resolve([Buffer.alloc(0)]) as Promise; + }; + + sandbox.stub(bucket, 'file').callsFake(filename => { + const file = new File(bucket, filename); + file.download = download; + return file; + }); + await transferManager.downloadManyFiles(filesOrFolder, { + prefix: prefix, + }); + assert.strictEqual( + mkdirSpy.calledOnceWith(expectedDir, { + recursive: true, + }), + true + ); + }); }); describe('downloadFileInChunks', () => {