feat(storage): enable CRC32C validation by default in transfer manager (#8350)

* feat: enable CRC32C validation by default in transfer manager and disable it for individual shard downloads

* feat: update validation option to support boolean values and improve default handling logic

* refactor: update validation property type definition to restrict string input to crc32c

* refactor(storage): use sandbox stubs for file.download in transfer-manager tests

Author: thiyaguk09

handwritten/storage/src/transfer-manager.ts

@@ -117,7 +117,7 @@ export interface DownloadFileInChunksOptions {
   concurrencyLimit?: number;
   chunkSizeBytes?: number;
   destination?: string;
-  validation?: 'crc32c' | false;
+  validation?: 'crc32c' | boolean;
   noReturnData?: boolean;
 }
 
@@ -736,7 +736,7 @@ export class TransferManager {
    * @property {number} [concurrencyLimit] The number of concurrently executing promises
    * 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 {'crc32c' | boolean} [validation] Whether or not to perform a CRC32C validation check when download is complete. Defaults to 'crc32c'.
    * @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.
    *
    */
@@ -757,10 +757,19 @@ export class TransferManager {
    *
    * //-
    * // Download a large file in chunks utilizing parallel operations.
+   * // CRC32C validation is performed by default.
    * //-
    * const response = await transferManager.downloadFileInChunks(bucket.file('large-file.txt');
    * // Your local directory now contains:
    * // - "large-file.txt" (with the contents from my-bucket.large-file.txt)
+   *
+   * //-
+   * // To disable validation:
+   * //-
+   * const responseWithoutValidation = await transferManager.downloadFileInChunks(
+   *   bucket.file('large-file.txt'),
+   *   { validation: false }
+   * );
    * ```
    *
    */
@@ -780,6 +789,12 @@ export class TransferManager {
         ? this.bucket.file(fileOrName)
         : fileOrName;
 
+    // Default validation to 'crc32c' if undefined or true, otherwise respect user's value
+    const validation =
+      options.validation === undefined || options.validation === true
+        ? 'crc32c'
+        : options.validation;
+
     const fileInfo = await file.get();
     const size = parseInt(fileInfo[0].metadata.size!.toString());
     // If the file size does not meet the threshold download it as a single chunk.
@@ -801,6 +816,7 @@ export class TransferManager {
             start: chunkStart,
             end: chunkEnd,
             [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_SHARDED,
+            validation: false, // Disable validation on individual chunks
           });
           const result = await fileToWrite.write(
             resp[0],
@@ -823,7 +839,8 @@ export class TransferManager {
       await fileToWrite.close();
     }
 
-    if (options.validation === 'crc32c' && fileInfo[0].metadata.crc32c) {
+    // Check against the defaulted validation option
+    if (validation === 'crc32c' && fileInfo[0].metadata.crc32c) {
       const downloadedCrc32C = await CRC32C.fromFile(filePath);
       if (!downloadedCrc32C.validate(fileInfo[0].metadata.crc32c)) {
         const mismatchError = new RequestError(
@@ -833,6 +850,7 @@ export class TransferManager {
         throw mismatchError;
       }
     }
+
     if (noReturnData) return;
     return [Buffer.concat(chunks as Buffer[], size)];
   }

handwritten/storage/test/transfer-manager.ts

@@ -562,13 +562,16 @@ describe('Transfer Manager', () => {
 
   describe('downloadFileInChunks', () => {
     let file: File;
+    let fromFileStub: sinon.SinonStub;
 
     beforeEach(() => {
       sandbox.stub(fsp, 'open').resolves({
         close: () => Promise.resolve(),
         write: (buffer: unknown) => Promise.resolve({buffer}),
       } as fsp.FileHandle);
 
+      fromFileStub = sandbox.stub(CRC32C, 'fromFile').resolves(new CRC32C(0));
+
       file = new File(bucket, 'some-large-file');
       sandbox.stub(file, 'get').resolves([
         {
@@ -612,26 +615,41 @@ describe('Transfer Manager', () => {
     });
 
     it('should call fromFile when validation is set to crc32c', async () => {
-      let callCount = 0;
-      file.download = () => {
-        return Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
-      };
-      CRC32C.fromFile = () => {
-        callCount++;
-        return Promise.resolve(new CRC32C(0));
-      };
+      sandbox.stub(file, 'download').resolves([Buffer.alloc(0)]);
 
       await transferManager.downloadFileInChunks(file, {validation: 'crc32c'});
-      assert.strictEqual(callCount, 1);
+      assert.strictEqual(fromFileStub.callCount, 1);
+    });
+
+    it('should perform CRC32C validation by default', async () => {
+      sandbox.stub(file, 'download').resolves([Buffer.alloc(0)]);
+
+      await transferManager.downloadFileInChunks(file);
+      assert.strictEqual(fromFileStub.callCount, 1);
+    });
+
+    it('should not perform validation when validation is false', async () => {
+      sandbox.stub(file, 'download').resolves([Buffer.alloc(0)]);
+
+      await transferManager.downloadFileInChunks(file, {validation: false});
+      assert.strictEqual(fromFileStub.callCount, 0);
+    });
+
+    it('should disable individual sharded chunk validation in download calls', async () => {
+      let shardedValidationOption: any = undefined;
+      sandbox.stub(file, 'download').callsFake(async options => {
+        shardedValidationOption = (options as DownloadOptions).validation;
+        return [Buffer.alloc(100)];
+      });
+
+      await transferManager.downloadFileInChunks(file);
+      assert.strictEqual(shardedValidationOption, false);
     });
 
     it('should throw an error if crc32c validation fails', async () => {
-      file.download = () => {
-        return Promise.resolve([Buffer.alloc(0)]) as Promise<DownloadResponse>;
-      };
-      CRC32C.fromFile = () => {
-        return Promise.resolve(new CRC32C(1)); // Set non-expected initial value
-      };
+      sandbox.stub(file, 'download').resolves([Buffer.alloc(0)]);
+
+      fromFileStub.resolves(new CRC32C(1)); // Set non-expected initial value
 
       await assert.rejects(
         transferManager.downloadFileInChunks(file, {validation: 'crc32c'}),