From 1e613e6d94fd54d1cdb296fbaf9ad54f69a3aee9 Mon Sep 17 00:00:00 2001 From: Rich Hodgkins Date: Tue, 28 Nov 2023 17:15:56 +0000 Subject: [PATCH 1/7] First pass at fixes for #2372 --- src/transfer-manager.ts | 45 ++++++++++++++++++++++++++++++---------- test/transfer-manager.ts | 1 + 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index 38966a0bd..a909a4750 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -15,7 +15,13 @@ */ import {Bucket, UploadOptions, UploadResponse} from './bucket.js'; -import {DownloadOptions, DownloadResponse, File} from './file.js'; +import { + DownloadOptions, + DownloadResponse, + File, + FileExceptionMessages, + RequestError, +} from './file.js'; import pLimit from 'p-limit'; import * as path from 'path'; import {createReadStream, promises as fsp} from 'fs'; @@ -105,6 +111,7 @@ export interface DownloadFileInChunksOptions { chunkSizeBytes?: number; destination?: string; validation?: 'crc32c' | false; + noReturnData?: true; } export interface UploadFileInChunksOptions { @@ -639,7 +646,8 @@ export class TransferManager { let limit = pLimit( options.concurrencyLimit || DEFAULT_PARALLEL_CHUNKED_DOWNLOAD_LIMIT ); - const promises: Promise<{bytesWritten: number; buffer: Buffer}>[] = []; + const noReturnData = Boolean(options.noReturnData); + const promises: Promise[] = []; const file: File = typeof fileOrName === 'string' ? this.bucket.file(fileOrName) @@ -667,24 +675,39 @@ export class TransferManager { end: chunkEnd, [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_SHARDED, }); - return fileToWrite.write(resp[0], 0, resp[0].length, chunkStart); + const result = await fileToWrite.write( + resp[0], + 0, + resp[0].length, + chunkStart + ); + if (noReturnData) return; + return result.buffer; }) ); start += chunkSize; } - let results: DownloadResponse; + let chunks: Array; try { - const data = await Promise.all(promises); - results = data.map(result => result.buffer) as DownloadResponse; - if (options.validation === 'crc32c') { - await CRC32C.fromFile(filePath); - } - return results; + chunks = await Promise.all(promises); } finally { - fileToWrite.close(); + await fileToWrite.close(); + } + + if (options.validation === 'crc32c' && fileInfo[0].metadata.crc32c) { + const downloadedCrc32C = await CRC32C.fromFile(filePath); + if (!downloadedCrc32C.validate(fileInfo[0].metadata.crc32c)) { + const mismatchError = new RequestError( + FileExceptionMessages.DOWNLOAD_MISMATCH + ); + mismatchError.code = 'CONTENT_DOWNLOAD_MISMATCH'; + throw mismatchError; + } } + if (noReturnData) return; + return [Buffer.concat(chunks as Buffer[], size)]; } /** diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 5e58f7119..bc7c3c8f2 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -249,6 +249,7 @@ describe('Transfer Manager', () => { { metadata: { size: 1024, + crc32c: 'AAAAAA==', }, }, ]); From b9fc725a14dd3db63df6e22cd1be555976a7b2b7 Mon Sep 17 00:00:00 2001 From: Rich Hodgkins Date: Tue, 28 Nov 2023 18:21:45 +0000 Subject: [PATCH 2/7] Added JSDoc for DownloadFileInChunksOptions#noReturnData --- src/transfer-manager.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index a909a4750..dcf4f749b 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -611,6 +611,7 @@ export class TransferManager { * to use when downloading the file. * @property {number} [chunkSizeBytes] The size in bytes of each chunk to be downloaded. * @property {string | boolean} [validation] Whether or not to perform a CRC32C validation check when download is complete. + * @property {boolean} [noReturnData] Wether or not to return the downloaded data. A `true` value here would be useful for files with a size that will not fit into memory. * */ /** From 87f5f491d821fc980254706dd2b84832be138e3f Mon Sep 17 00:00:00 2001 From: Rich Hodgkins Date: Tue, 28 Nov 2023 18:29:35 +0000 Subject: [PATCH 3/7] Changed type of DownloadFileInChunksOptions#noReturnData to be clearer on the default --- 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 dcf4f749b..fe82fbeec 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -111,7 +111,7 @@ export interface DownloadFileInChunksOptions { chunkSizeBytes?: number; destination?: string; validation?: 'crc32c' | false; - noReturnData?: true; + noReturnData?: boolean; } export interface UploadFileInChunksOptions { From 2310d72f9cebdf51a84bf0e8e8c551726049a066 Mon Sep 17 00:00:00 2001 From: Rich Hodgkins Date: Wed, 29 Nov 2023 07:08:07 +0000 Subject: [PATCH 4/7] Update src/transfer-manager.ts Fixed typo Co-authored-by: Daniel Bankhead --- 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 fe82fbeec..a3555a41c 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -611,7 +611,7 @@ export class TransferManager { * to use when downloading the file. * @property {number} [chunkSizeBytes] The size in bytes of each chunk to be downloaded. * @property {string | boolean} [validation] Whether or not to perform a CRC32C validation check when download is complete. - * @property {boolean} [noReturnData] Wether or not to return the downloaded data. A `true` value here would be useful for files with a size that will not fit into memory. + * @property {boolean} [noReturnData] Whether or not to return the downloaded data. A `true` value here would be useful for files with a size that will not fit into memory. * */ /** From 5045acfa8c1d4c77ae62d52ebd1ab7b17c3ae959 Mon Sep 17 00:00:00 2001 From: Rich Hodgkins Date: Wed, 29 Nov 2023 07:22:14 +0000 Subject: [PATCH 5/7] TransferManager#downloadFileInChunks test case for failed CRC32C validation --- test/transfer-manager.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index bc7c3c8f2..68aafb06c 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -280,6 +280,22 @@ describe('Transfer Manager', () => { assert.strictEqual(callCount, 1); }); + it('should throw an error if crc32c validation fails', async () => { + file.download = () => { + return Promise.resolve([Buffer.alloc(0)]) as Promise; + }; + CRC32C.fromFile = () => { + return Promise.resolve(new CRC32C(1)); // Set non-expected initial value + }; + + await assert.rejects( + transferManager.downloadFileInChunks(file, {validation: 'crc32c'}), + { + code: 'CONTENT_DOWNLOAD_MISMATCH', + } + ); + }); + it('should set the appropriate `GCCL_GCS_CMD_KEY`', async () => { sandbox.stub(file, 'download').callsFake(async options => { assert.strictEqual( From 6bcd503146d9af6cc3f869c1219c906ad3fbfd23 Mon Sep 17 00:00:00 2001 From: Rich Hodgkins Date: Wed, 29 Nov 2023 07:30:14 +0000 Subject: [PATCH 6/7] TransferManager#downloadFileInChunks test case for return values and noReturnData option --- test/transfer-manager.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 68aafb06c..4c5d3b50f 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -266,6 +266,26 @@ describe('Transfer Manager', () => { assert.strictEqual(downloadCallCount, 1); }); + it('should return downloaded data', async () => { + sandbox.stub(file, 'download').callsFake(() => { + return Promise.resolve([Buffer.alloc(100)]); + }); + + const data = await transferManager.downloadFileInChunks(file); + assert.deepStrictEqual(data, [Buffer.alloc(1024)]); + }); + + it('should not return downloaded data when noReturnData flag is set', async () => { + sandbox.stub(file, 'download').callsFake(() => { + return Promise.resolve([Buffer.alloc(100)]); + }); + + const data = await transferManager.downloadFileInChunks(file, { + noReturnData: true, + }); + assert.strictEqual(data, undefined); + }); + it('should call fromFile when validation is set to crc32c', async () => { let callCount = 0; file.download = () => { From fba246c72780d71c9e14f249ed168595845c09bd Mon Sep 17 00:00:00 2001 From: Rich Hodgkins Date: Wed, 29 Nov 2023 08:45:22 +0000 Subject: [PATCH 7/7] Fixed JSDocs for TransferManager#downloadFileInChunks --- src/transfer-manager.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index a3555a41c..37230389f 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -618,9 +618,9 @@ export class TransferManager { * Download a large file in chunks utilizing parallel download operations. This is a convenience method * that utilizes {@link File#download} to perform the download. * - * @param {object} [file | string] {@link File} to download. + * @param {File | string} fileOrName {@link File} to download. * @param {DownloadFileInChunksOptions} [options] Configuration options. - * @returns {Promise} + * @returns {Promise} * * @example * ```