From 2c2dcc6e9e6adcd2613d7ab42a979bd79ce5a555 Mon Sep 17 00:00:00 2001 From: Wai Ho Choy Date: Thu, 18 May 2023 00:29:57 -0700 Subject: [PATCH 01/14] fix: allow files in nested folders to be downloaded onto local machine --- src/file.ts | 3 +++ src/transfer-manager.ts | 14 +++++++------- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/file.ts b/src/file.ts index 1237fd01d..a4ba675eb 100644 --- a/src/file.ts +++ b/src/file.ts @@ -34,6 +34,7 @@ import * as resumableUpload from './resumable-upload'; import {Writable, Readable, pipeline, Transform, PassThrough} from 'stream'; import * as zlib from 'zlib'; import * as http from 'http'; +import * as path from 'path'; import { ExceptionMessages, @@ -2098,6 +2099,8 @@ class File extends ServiceObject { const fileStream = this.createReadStream(options); let receivedData = false; if (destination) { + fs.mkdirSync(path.dirname(destination), {recursive: true}); + fileStream .on('error', callback) .once('data', data => { diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index 58cd75c11..d1b1b2dff 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -238,6 +238,8 @@ export class TransferManager { prefix: filesOrFolder, }); files = directoryFiles[0]; + // filter out directories + files = files.filter(file => !file.name.endsWith('/')); } else { files = filesOrFolder.map(curFile => { if (typeof curFile === 'string') { @@ -258,13 +260,11 @@ export class TransferManager { {}, options.passthroughOptions ); - if (options.prefix) { - passThroughOptionsCopy.destination = path.join( - options.prefix || '', - passThroughOptionsCopy.destination || '', - file.name - ); - } + passThroughOptionsCopy.destination = path.join( + options.prefix || '', + passThroughOptionsCopy.destination || '', + file.name + ); if (options.stripPrefix) { passThroughOptionsCopy.destination = file.name.replace(regex, ''); } From 4cfce7f1b75bcac77332c62f8bcf41637a9c1c94 Mon Sep 17 00:00:00 2001 From: Wai Ho Choy Date: Sat, 27 May 2023 19:05:06 -0700 Subject: [PATCH 02/14] fix: revert removal of options.prefix check --- src/transfer-manager.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index d1b1b2dff..bf4fa9b06 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -260,11 +260,14 @@ export class TransferManager { {}, options.passthroughOptions ); - passThroughOptionsCopy.destination = path.join( - options.prefix || '', - passThroughOptionsCopy.destination || '', - file.name - ); + + if (options.prefix) { + passThroughOptionsCopy.destination = path.join( + options.prefix || '', + passThroughOptionsCopy.destination || '', + file.name + ); + } if (options.stripPrefix) { passThroughOptionsCopy.destination = file.name.replace(regex, ''); } From dff1e5088021def2f720b9d0cffc1254534ee6d7 Mon Sep 17 00:00:00 2001 From: Wai Ho Choy Date: Sun, 28 May 2023 18:35:40 -0700 Subject: [PATCH 03/14] test: file.download should create directory paths recursively --- test/file.ts | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/file.ts b/test/file.ts index 14b493f22..4f6674fe6 100644 --- a/test/file.ts +++ b/test/file.ts @@ -35,6 +35,7 @@ import * as resumableUpload from '../src/resumable-upload'; import * as sinon from 'sinon'; import * as tmp from 'tmp'; import * as zlib from 'zlib'; +import * as path from 'path'; import { Bucket, @@ -2678,6 +2679,34 @@ describe('File', () => { }); }); }); + + it('should recursively create directory and write file contents if destination path is nested', 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.ifError(err); + assert(fs.existsSync(nestedPath)); + fs.readFile(nestedPath, (err, tmpFileContents) => { + assert.ifError(err); + assert.strictEqual(fileContents, tmpFileContents.toString()); + done(); + }); + }); + }); + }); }); }); From e7039e107c2fb6faab8fc95c5852864ac64af16a Mon Sep 17 00:00:00 2001 From: Wai Ho Choy Date: Mon, 29 May 2023 02:32:24 -0700 Subject: [PATCH 04/14] fix: move the filtering of directory objects from TransferManager.downloadManyFiles to File.download --- src/file.ts | 11 +++++++- src/transfer-manager.ts | 3 --- test/file.ts | 56 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/src/file.ts b/src/file.ts index a4ba675eb..158673505 100644 --- a/src/file.ts +++ b/src/file.ts @@ -2098,7 +2098,16 @@ class File extends ServiceObject { const fileStream = this.createReadStream(options); let receivedData = false; - if (destination) { + + // Skip directory objects as they cannot be written to local filesystem + /** + * Temporary comment: + * Could consider returning an error, but TransferManager will have to handle rejected promises in Promises.all. + * Example: Promise.allSettled(promises) or Promise.all(promises.map(p => p.catch(e => e))) + */ + if (destination && destination.endsWith('/')) { + callback?.(null, Buffer.alloc(0)); + } else if (destination) { fs.mkdirSync(path.dirname(destination), {recursive: true}); fileStream diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index bf4fa9b06..68740d817 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -238,8 +238,6 @@ export class TransferManager { prefix: filesOrFolder, }); files = directoryFiles[0]; - // filter out directories - files = files.filter(file => !file.name.endsWith('/')); } else { files = filesOrFolder.map(curFile => { if (typeof curFile === 'string') { @@ -273,7 +271,6 @@ export class TransferManager { } promises.push(limit(() => file.download(passThroughOptionsCopy))); } - return Promise.all(promises); } diff --git a/test/file.ts b/test/file.ts index 4f6674fe6..78db6c766 100644 --- a/test/file.ts +++ b/test/file.ts @@ -2556,6 +2556,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) => { @@ -2698,7 +2704,7 @@ describe('File', () => { file.download({destination: nestedPath}, (err: Error) => { assert.ifError(err); - assert(fs.existsSync(nestedPath)); + assert.strictEqual(fs.existsSync(nestedPath), true); fs.readFile(nestedPath, (err, tmpFileContents) => { assert.ifError(err); assert.strictEqual(fileContents, tmpFileContents.toString()); @@ -2707,6 +2713,54 @@ describe('File', () => { }); }); }); + + it('should skip write if asked to write a directory object', done => { + // Temporary comment: Option 1: Spy on fs methods + const mkdirSync = sandbox.spy(fakeFs, 'mkdirSync'); + const createWriteStream = sandbox.spy(fakeFs, 'createWriteStream'); + + tmp.setGracefulCleanup(); + tmp.dir(async (err, tmpDirPath) => { + assert.ifError(err); + + const fileContents = ''; + + Object.assign(fileReadStream, { + _read(this: Readable) { + this.push(fileContents); + this.push(null); + }, + }); + + const nestedDir = path.join(tmpDirPath, 'a', 'b', 'c', '/'); + + file.download( + {destination: nestedDir}, + (err: Error, buffer: Buffer) => { + try { + assert.ifError(err); + + // Temporary comment: Option 1: Spy on fs methods + assert.strictEqual(createWriteStream.callCount, 0); + assert.strictEqual(mkdirSync.callCount, 0); + + // Temporary comment: Option 2: Verify that no folder and/or file were created + assert.strictEqual( + fs.existsSync(path.dirname(nestedDir)), + false + ); + assert.strictEqual(fs.existsSync(nestedDir), false); + + // Temporary comment: Not very useful since no user would care about an empty buffer. Could remove. + assert.strictEqual(buffer.equals(Buffer.alloc(0)), true); + done(); + } catch (e) { + done(e); + } + } + ); + }); + }); }); }); From 0218cdbefaacbe6d4044126a4ecb0c8e8d545de0 Mon Sep 17 00:00:00 2001 From: Wai Ho Choy Date: Tue, 30 May 2023 15:46:23 -0700 Subject: [PATCH 05/14] fix: handle windows path --- src/file.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/file.ts b/src/file.ts index 158673505..fa9b4f829 100644 --- a/src/file.ts +++ b/src/file.ts @@ -2105,7 +2105,10 @@ class File extends ServiceObject { * Could consider returning an error, but TransferManager will have to handle rejected promises in Promises.all. * Example: Promise.allSettled(promises) or Promise.all(promises.map(p => p.catch(e => e))) */ - if (destination && destination.endsWith('/')) { + if ( + destination && + (destination.endsWith('/') || destination.endsWith('\\')) + ) { callback?.(null, Buffer.alloc(0)); } else if (destination) { fs.mkdirSync(path.dirname(destination), {recursive: true}); From f34b30fbc6ecbef20b72f27313ca45fff06e9386 Mon Sep 17 00:00:00 2001 From: Wai Ho Choy Date: Sat, 3 Jun 2023 08:49:07 -0700 Subject: [PATCH 06/14] chore: remove temporary comments --- src/file.ts | 5 ----- src/transfer-manager.ts | 2 +- test/file.ts | 12 ------------ 3 files changed, 1 insertion(+), 18 deletions(-) diff --git a/src/file.ts b/src/file.ts index fa9b4f829..9a1f02fe9 100644 --- a/src/file.ts +++ b/src/file.ts @@ -2100,11 +2100,6 @@ class File extends ServiceObject { let receivedData = false; // Skip directory objects as they cannot be written to local filesystem - /** - * Temporary comment: - * Could consider returning an error, but TransferManager will have to handle rejected promises in Promises.all. - * Example: Promise.allSettled(promises) or Promise.all(promises.map(p => p.catch(e => e))) - */ if ( destination && (destination.endsWith('/') || destination.endsWith('\\')) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index 68740d817..58cd75c11 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -258,7 +258,6 @@ export class TransferManager { {}, options.passthroughOptions ); - if (options.prefix) { passThroughOptionsCopy.destination = path.join( options.prefix || '', @@ -271,6 +270,7 @@ export class TransferManager { } promises.push(limit(() => file.download(passThroughOptionsCopy))); } + return Promise.all(promises); } diff --git a/test/file.ts b/test/file.ts index 78db6c766..ad1b91044 100644 --- a/test/file.ts +++ b/test/file.ts @@ -2715,7 +2715,6 @@ describe('File', () => { }); it('should skip write if asked to write a directory object', done => { - // Temporary comment: Option 1: Spy on fs methods const mkdirSync = sandbox.spy(fakeFs, 'mkdirSync'); const createWriteStream = sandbox.spy(fakeFs, 'createWriteStream'); @@ -2739,19 +2738,8 @@ describe('File', () => { (err: Error, buffer: Buffer) => { try { assert.ifError(err); - - // Temporary comment: Option 1: Spy on fs methods assert.strictEqual(createWriteStream.callCount, 0); assert.strictEqual(mkdirSync.callCount, 0); - - // Temporary comment: Option 2: Verify that no folder and/or file were created - assert.strictEqual( - fs.existsSync(path.dirname(nestedDir)), - false - ); - assert.strictEqual(fs.existsSync(nestedDir), false); - - // Temporary comment: Not very useful since no user would care about an empty buffer. Could remove. assert.strictEqual(buffer.equals(Buffer.alloc(0)), true); done(); } catch (e) { From 41fa9cb88763a014d5275d5ddfb7d3dd2cdaad15 Mon Sep 17 00:00:00 2001 From: Wai Ho Choy Date: Sat, 3 Jun 2023 09:40:10 -0700 Subject: [PATCH 07/14] test: add scenarios where destination is set in Transfer Manager --- src/transfer-manager.ts | 7 +++++-- test/transfer-manager.ts | 29 +++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index 58cd75c11..dd5e11627 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -197,7 +197,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 @@ -258,7 +260,7 @@ export class TransferManager { {}, options.passthroughOptions ); - if (options.prefix) { + if (options.prefix || passThroughOptionsCopy.destination) { passThroughOptionsCopy.destination = path.join( options.prefix || '', passThroughOptionsCopy.destination || '', @@ -268,6 +270,7 @@ export class TransferManager { if (options.stripPrefix) { passThroughOptionsCopy.destination = file.name.replace(regex, ''); } + console.warn(passThroughOptionsCopy); promises.push(limit(() => file.download(passThroughOptionsCopy))); } diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index be9d56cd2..18bde7cf5 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -262,6 +262,35 @@ describe('Transfer Manager', () => { file.download = download; await transferManager.downloadManyFiles([file], {stripPrefix}); }); + + 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 = (options: DownloadOptions) => { + assert.strictEqual(options.destination, expectedDestination); + }; + + 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 = (options: DownloadOptions) => { + assert.strictEqual(options.destination, undefined); + }; + + const file = new File(bucket, filename); + file.download = download; + await transferManager.downloadManyFiles([file], options); + }); }); describe('downloadFileInChunks', () => { From 44aa7e08bba1def5366919bb9dd8b57e2d02528b Mon Sep 17 00:00:00 2001 From: Wai Ho Choy Date: Sat, 3 Jun 2023 09:40:40 -0700 Subject: [PATCH 08/14] chore: remove console.warn --- src/transfer-manager.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index dd5e11627..0bb2a3cb8 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -270,7 +270,6 @@ export class TransferManager { if (options.stripPrefix) { passThroughOptionsCopy.destination = file.name.replace(regex, ''); } - console.warn(passThroughOptionsCopy); promises.push(limit(() => file.download(passThroughOptionsCopy))); } From da9f4f3dbba7e0819a31ce88813eaa04f691a227 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Mon, 6 May 2024 10:24:54 +0000 Subject: [PATCH 09/14] fix: merge with main --- src/transfer-manager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index ecca8b573..33c43b518 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -589,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 || '', From 6bb184137ae0affad3a514de330c72b3d3b8bb76 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 8 May 2024 20:56:55 +0000 Subject: [PATCH 10/14] fix: added transfer manager tests for prefix --- test/transfer-manager.ts | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 4c5d3b50f..477a394f6 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,45 @@ 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); + }); }); describe('downloadFileInChunks', () => { From 303f3fb870e0f9b3561bf30de93ee957ac23e193 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 15 May 2024 12:02:22 +0000 Subject: [PATCH 11/14] fix: address PR comments --- src/file.ts | 13 ++--------- src/transfer-manager.ts | 17 ++++++++++++-- test/file.ts | 49 ++++------------------------------------ test/transfer-manager.ts | 31 +++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 58 deletions(-) diff --git a/src/file.ts b/src/file.ts index 188759c6a..32f1f62b7 100644 --- a/src/file.ts +++ b/src/file.ts @@ -31,7 +31,6 @@ import * as resumableUpload from './resumable-upload.js'; import {Writable, Readable, pipeline, Transform, PipelineSource} from 'stream'; import * as zlib from 'zlib'; import * as http from 'http'; -import * as path from 'path'; import { ExceptionMessages, @@ -2290,20 +2289,12 @@ class File extends ServiceObject { const fileStream = this.createReadStream(options); let receivedData = false; - // Skip directory objects as they cannot be written to local filesystem - if ( - destination && - (destination.endsWith('/') || destination.endsWith('\\')) - ) { - callback?.(null, Buffer.alloc(0)); - } else if (destination) { - fs.mkdirSync(path.dirname(destination), {recursive: true}); - + 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 33c43b518..20e6cb2b6 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -24,7 +24,7 @@ import { } from './file.js'; import pLimit from 'p-limit'; import * as path from 'path'; -import {createReadStream, promises as fsp} from 'fs'; +import {createReadStream, mkdirSync, promises as fsp} from 'fs'; import {CRC32C} from './crc32c.js'; import {GoogleAuth} from 'google-auth-library'; import {XMLParser, XMLBuilder} from 'fast-xml-parser'; @@ -600,7 +600,20 @@ 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)) { + mkdirSync(destination, {recursive: true}); + // Skip directory objects as they cannot be written to local filesystem + 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 226771893..5369083af 100644 --- a/test/file.ts +++ b/test/file.ts @@ -28,12 +28,12 @@ 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'; import * as tmp from 'tmp'; import * as zlib from 'zlib'; -import * as path from 'path'; import { Bucket, @@ -2702,7 +2702,7 @@ describe('File', () => { }); }); - it('should recursively create directory and write file contents if destination path is nested', done => { + it('should fail if provided destination directory does not exist', done => { tmp.setGracefulCleanup(); tmp.dir(async (err, tmpDirPath) => { assert.ifError(err); @@ -2719,50 +2719,9 @@ describe('File', () => { const nestedPath = path.join(tmpDirPath, 'a', 'b', 'c', 'file.txt'); file.download({destination: nestedPath}, (err: Error) => { - assert.ifError(err); - assert.strictEqual(fs.existsSync(nestedPath), true); - fs.readFile(nestedPath, (err, tmpFileContents) => { - assert.ifError(err); - assert.strictEqual(fileContents, tmpFileContents.toString()); - done(); - }); - }); - }); - }); - - it('should skip write if asked to write a directory object', done => { - const mkdirSync = sandbox.spy(fakeFs, 'mkdirSync'); - const createWriteStream = sandbox.spy(fakeFs, 'createWriteStream'); - - tmp.setGracefulCleanup(); - tmp.dir(async (err, tmpDirPath) => { - assert.ifError(err); - - const fileContents = ''; - - Object.assign(fileReadStream, { - _read(this: Readable) { - this.push(fileContents); - this.push(null); - }, + assert.ok(err); + done(); }); - - const nestedDir = path.join(tmpDirPath, 'a', 'b', 'c', '/'); - - file.download( - {destination: nestedDir}, - (err: Error, buffer: Buffer) => { - try { - assert.ifError(err); - assert.strictEqual(createWriteStream.callCount, 0); - assert.strictEqual(mkdirSync.callCount, 0); - assert.strictEqual(buffer.equals(Buffer.alloc(0)), true); - done(); - } catch (e) { - done(e); - } - } - ); }); }); }); diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 477a394f6..d26ba1692 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -273,6 +273,37 @@ describe('Transfer Manager', () => { file.download = download; await transferManager.downloadManyFiles([file], options); }); + + it('should recursively create directory and write file contents if destination path is nested', async () => { + const filesOrFolder = ['nestedFolder/', 'nestedFolder/first.txt']; + const mkdirSyncSpy = sandbox.spy(fs, 'mkdirSync'); + const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { + if (typeof optionsOrCb === 'function') { + optionsOrCb(null, Buffer.alloc(0)); + } else if (optionsOrCb) { + assert.strictEqual( + optionsOrCb.destination, + 'test-prefix/nestedFolder/first.txt' + ); + } + 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: 'test-prefix', + }); + assert.strictEqual( + mkdirSyncSpy.calledOnceWith('test-prefix/nestedFolder/', { + recursive: true, + }), + true + ); + }); }); describe('downloadFileInChunks', () => { From 63cfcebbb6e0f1a944185bebde67fe473c5ee6d5 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 15 May 2024 12:52:27 +0000 Subject: [PATCH 12/14] fix: paths for windows ci --- test/transfer-manager.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index d26ba1692..d4e5f7985 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -275,16 +275,18 @@ describe('Transfer Manager', () => { }); it('should recursively create directory and write file contents if destination path is nested', async () => { - const filesOrFolder = ['nestedFolder/', 'nestedFolder/first.txt']; + 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 mkdirSyncSpy = sandbox.spy(fs, 'mkdirSync'); const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { if (typeof optionsOrCb === 'function') { optionsOrCb(null, Buffer.alloc(0)); } else if (optionsOrCb) { - assert.strictEqual( - optionsOrCb.destination, - 'test-prefix/nestedFolder/first.txt' - ); + assert.strictEqual(optionsOrCb.destination, expectedFilePath); } return Promise.resolve([Buffer.alloc(0)]) as Promise; }; @@ -295,10 +297,10 @@ describe('Transfer Manager', () => { return file; }); await transferManager.downloadManyFiles(filesOrFolder, { - prefix: 'test-prefix', + prefix: prefix, }); assert.strictEqual( - mkdirSyncSpy.calledOnceWith('test-prefix/nestedFolder/', { + mkdirSyncSpy.calledOnceWith(expectedDir, { recursive: true, }), true From d886548eae163dcd6224e0f0a627475b090bbb05 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Sat, 18 May 2024 07:15:35 +0000 Subject: [PATCH 13/14] fix: migrate mkdirsync to await fs/promises/mkdir --- src/transfer-manager.ts | 5 ++--- test/transfer-manager.ts | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index 20e6cb2b6..d0fe21f75 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -24,7 +24,7 @@ import { } from './file.js'; import pLimit from 'p-limit'; import * as path from 'path'; -import {createReadStream, mkdirSync, promises as fsp} from 'fs'; +import {createReadStream, promises as fsp} from 'fs'; import {CRC32C} from './crc32c.js'; import {GoogleAuth} from 'google-auth-library'; import {XMLParser, XMLBuilder} from 'fast-xml-parser'; @@ -604,8 +604,7 @@ export class TransferManager { limit(async () => { const destination = passThroughOptionsCopy.destination; if (destination && destination.endsWith(path.sep)) { - mkdirSync(destination, {recursive: true}); - // Skip directory objects as they cannot be written to local filesystem + await fsp.mkdir(destination, {recursive: true}); return Promise.resolve([ Buffer.alloc(0), ]) as Promise; diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index d4e5f7985..36f7f9e4e 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -281,7 +281,7 @@ describe('Transfer Manager', () => { const filesOrFolder = [folder, path.join(folder, file)]; const expectedFilePath = path.join(prefix, folder, file); const expectedDir = path.join(prefix, folder); - const mkdirSyncSpy = sandbox.spy(fs, 'mkdirSync'); + const mkdirSyncSpy = sandbox.spy(fsp, 'mkdir'); const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { if (typeof optionsOrCb === 'function') { optionsOrCb(null, Buffer.alloc(0)); From a9419a525c0c116c53c90db17d780eeaadc2a38d Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Sat, 18 May 2024 07:20:41 +0000 Subject: [PATCH 14/14] fix: var name --- test/transfer-manager.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 36f7f9e4e..741e0a91c 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -281,7 +281,7 @@ describe('Transfer Manager', () => { const filesOrFolder = [folder, path.join(folder, file)]; const expectedFilePath = path.join(prefix, folder, file); const expectedDir = path.join(prefix, folder); - const mkdirSyncSpy = sandbox.spy(fsp, 'mkdir'); + const mkdirSpy = sandbox.spy(fsp, 'mkdir'); const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { if (typeof optionsOrCb === 'function') { optionsOrCb(null, Buffer.alloc(0)); @@ -300,7 +300,7 @@ describe('Transfer Manager', () => { prefix: prefix, }); assert.strictEqual( - mkdirSyncSpy.calledOnceWith(expectedDir, { + mkdirSpy.calledOnceWith(expectedDir, { recursive: true, }), true