From 56c745ba98b5d82f9f0b4ecedc9f563eef01925a Mon Sep 17 00:00:00 2001 From: Zach Bjornson Date: Mon, 31 Oct 2016 11:13:23 -0700 Subject: [PATCH] Change default hash to CRC32c and improve error message if MD5 requested but absent (#1750) --- AUTHORS | 1 + CONTRIBUTORS | 1 + packages/storage/src/file.js | 45 +++++++++++++++++++++++++---------- packages/storage/test/file.js | 45 +++++++++++++++++++++++++++++++---- 4 files changed, 75 insertions(+), 17 deletions(-) diff --git a/AUTHORS b/AUTHORS index 69d2765ed0f..52f7d4f7e5c 100644 --- a/AUTHORS +++ b/AUTHORS @@ -10,3 +10,4 @@ Google Inc. Anand Suresh Brett Bergmann Jesse Friedman +Zach Bjornson diff --git a/CONTRIBUTORS b/CONTRIBUTORS index cb4b8e85523..89f2a7bfff0 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -26,3 +26,4 @@ Patrick Costello Silvano Luciani Stephen Sawchuk Thomas Rognon +Zach Bjornson diff --git a/packages/storage/src/file.js b/packages/storage/src/file.js index 51265e16326..23a7e1e8397 100644 --- a/packages/storage/src/file.js +++ b/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 = [ diff --git a/packages/storage/test/file.js b/packages/storage/test/file.js index 8b685e8df22..fe9f24a0174 100644 --- a/packages/storage/test/file.js +++ b/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();