Change default hash to CRC32c and improve error message if MD5 reques…

…ted but absent (#1750)

AUTHORS

@@ -10,3 +10,4 @@ Google Inc.
 Anand Suresh
 Brett Bergmann
 Jesse Friedman
+Zach Bjornson <zbbjornson@gmail.com>

CONTRIBUTORS

@@ -26,3 +26,4 @@ Patrick Costello <pcostell@google.com>
 Silvano Luciani <silvano@google.com>
 Stephen Sawchuk <sawchuk@gmail.com>
 Thomas Rognon <tcrognon@gmail.com>
+Zach Bjornson <zbbjornson@gmail.com>

packages/storage/src/file.js

@@ -513,11 +513,11 @@ File.prototype.copy = function(destination, options, callback) {
  *
  * @param {object=} options - Configuration object.
  * @param {string|boolean} options.validation - Possible values: `"md5"`,
- *     `"crc32c"`, or `false`. By default, data integrity is validated with an
- *     MD5 checksum for maximum reliability, falling back to CRC32c when an MD5
- *     hash wasn't returned from the API. CRC32c will provide better performance
- *     with less reliability. You may also choose to skip validation completely,
- *     however this is **not recommended**.
+ *     `"crc32c"`, or `false`. By default, data integrity is validated with a
+ *     CRC32c checksum. You may use MD5 if preferred, but that hash is not
+ *     supported for composite objects. An error will be raised if MD5 is
+ *     specified but is not available. You may also choose to skip validation
+ *     completely, however this is **not recommended**.
  * @param {number} options.start - A byte offset to begin the file's download
  *     from. Default is 0. NOTE: Byte ranges are inclusive; that is,
  *     `options.start = 0` and `options.end = 999` represent the first 1000
@@ -581,13 +581,15 @@ File.prototype.createReadStream = function(options) {
   var tailRequest = options.end < 0;
   var throughStream = streamEvents(through());
 
-  var crc32c = options.validation !== false;
-  var md5 = options.validation !== false;
+  var crc32c = true;
+  var md5 = false;
 
   if (is.string(options.validation)) {
     options.validation = options.validation.toLowerCase();
     crc32c = options.validation === 'crc32c';
     md5 = options.validation === 'md5';
+  } else if (options.validation === false) {
+    crc32c = false;
   }
 
   if (rangeRequest) {
@@ -687,7 +689,15 @@ File.prototype.createReadStream = function(options) {
         failed = !validateStream.test('md5', hashes.md5);
       }
 
-      if (failed) {
+      if (md5 && !hashes.md5) {
+        var hashError = new Error([
+          'MD5 verification was specified, but is not available for the',
+          'requested object. MD5 is not available for composite objects.'
+        ].join(' '));
+        hashError.code = 'MD5_NOT_AVAILABLE';
+
+        throughStream.destroy(hashError);
+      } else if (failed) {
         var mismatchError = new Error([
           'The downloaded data did not match the data from the server.',
           'To be sure the content is the same, you should download the',
@@ -872,9 +882,10 @@ File.prototype.createResumableUpload = function(options, callback) {
  * @param {string} options.uri - The URI for an already-created resumable
  *     upload. See {module:storage/file#createResumableUpload}.
  * @param {string|boolean} options.validation - Possible values: `"md5"`,
- *     `"crc32c"`, or `false`. By default, data integrity is validated with an
- *     MD5 checksum for maximum reliability. CRC32c will provide better
- *     performance with less reliability. You may also choose to skip validation
+ *     `"crc32c"`, or `false`. By default, data integrity is validated with a
+ *     CRC32c checksum. You may use MD5 if preferred, but that hash is not
+ *     supported for composite objects. An error will be raised if MD5 is
+ *     specified but is not available. You may also choose to skip validation
  *     completely, however this is **not recommended**.
  * @return {WritableStream}
  *
@@ -945,13 +956,15 @@ File.prototype.createWriteStream = function(options) {
     options.metadata.contentEncoding = 'gzip';
   }
 
-  var crc32c = options.validation !== false;
-  var md5 = options.validation !== false;
+  var crc32c = true;
+  var md5 = false;
 
   if (is.string(options.validation)) {
     options.validation = options.validation.toLowerCase();
     crc32c = options.validation === 'crc32c';
     md5 = options.validation === 'md5';
+  } else if (options.validation === false) {
+    crc32c = false;
   }
 
   // Collect data as it comes in to store in a hash. This is compared to the
@@ -1024,6 +1037,12 @@ File.prototype.createWriteStream = function(options) {
             '\n\nThe delete attempt failed with this message:',
             '\n\n  ' + err.message
           ].join(' ');
+        } else if (md5 && !metadata.md5Hash) {
+          code = 'MD5_NOT_AVAILABLE';
+          message = [
+            'MD5 verification was specified, but is not available for the',
+            'requested object. MD5 is not available for composite objects.'
+          ].join(' ');
         } else {
           code = 'FILE_NO_UPLOAD';
           message = [

packages/storage/test/file.js

@@ -868,9 +868,9 @@ describe('File', function() {
           .resume();
       });
 
-      it('should default to md5 validation', function(done) {
+      it('should default to crc32c validation', function(done) {
         file.requestStream = getFakeSuccessfulRequest(data, {
-          headers: { 'x-goog-hash': 'md5=fakefakefake' }
+          headers: { 'x-goog-hash': 'crc32c=fakefakefake' }
         });
 
         file.createReadStream()
@@ -883,7 +883,7 @@ describe('File', function() {
 
       it('should ignore a data mismatch if validation: false', function(done) {
         file.requestStream = getFakeSuccessfulRequest(data, {
-          headers: { 'x-goog-hash': 'md5=fakefakefake' }
+          headers: { 'x-goog-hash': 'crc32c=fakefakefake' }
         });
 
         file.createReadStream({ validation: false })
@@ -896,7 +896,7 @@ describe('File', function() {
         it('should destroy after failed validation', function(done) {
           file.requestStream = getFakeSuccessfulRequest(
             'bad-data',
-            fakeResponse.crc32c
+            fakeResponse.md5
           );
 
           var readStream = file.createReadStream({ validation: 'md5' });
@@ -906,6 +906,20 @@ describe('File', function() {
           };
           readStream.resume();
         });
+
+        it('should destroy if MD5 is requested but absent', function(done) {
+          file.requestStream = getFakeSuccessfulRequest(
+            'bad-data',
+            fakeResponse.crc32c
+          );
+
+          var readStream = file.createReadStream({ validation: 'md5' });
+          readStream.destroy = function(err) {
+            assert.strictEqual(err.code, 'MD5_NOT_AVAILABLE');
+            done();
+          };
+          readStream.resume();
+        });
       });
     });
 
@@ -1317,6 +1331,29 @@ describe('File', function() {
         writable.end();
       });
 
+      it('should emit an error if MD5 is requested but absent', function(done) {
+        var writable = file.createWriteStream({validation: 'md5'});
+
+        file.startResumableUpload_ = function(stream) {
+          setImmediate(function() {
+            file.metadata = { crc32c: 'not-md5' };
+            stream.emit('complete');
+          });
+        };
+
+        file.delete = function(cb) {
+          cb();
+        };
+
+        writable.write(data);
+        writable.end();
+
+        writable.on('error', function(err) {
+          assert.equal(err.code, 'MD5_NOT_AVAILABLE');
+          done();
+        });
+      });
+
       it('should emit a different error if delete fails', function(done) {
         var writable = file.createWriteStream();