From b7148be1cfb9ef45499a784ed1683fe57550de55 Mon Sep 17 00:00:00 2001 From: Dave Gramlich Date: Mon, 14 Nov 2016 17:00:07 -0500 Subject: [PATCH 1/4] bigtable: promisify table#mutate --- package.json | 2 +- packages/bigtable/src/table.js | 49 ++++++------------- packages/bigtable/test/table.js | 83 +-------------------------------- 3 files changed, 15 insertions(+), 119 deletions(-) diff --git a/package.json b/package.json index fcb9f1300b4..b33586634f8 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "prepare-ghpages": "node ./scripts/docs/prepare.js", "remove-ghpages": "node ./scripts/docs/remove.js", "lint": "jshint scripts/ packages/ system-test/ test/ && jscs packages/ system-test/ test/", - "test": "npm run docs && npm run snippet-test && npm run unit-test", + "test": "rm -rf docs/json && npm run docs && npm run snippet-test && npm run unit-test", "unit-test": "mocha --timeout 5000 --bail packages/*/test/*.js", "snippet-test": "mocha --timeout 5000 --bail test/docs.js", "system-test": "mocha packages/*/system-test/*.js --no-timeouts --bail", diff --git a/packages/bigtable/src/table.js b/packages/bigtable/src/table.js index 7cc541e5435..8436d342699 100644 --- a/packages/bigtable/src/table.js +++ b/packages/bigtable/src/table.js @@ -734,16 +734,14 @@ Table.prototype.getRows = function(options, callback) { * * table.insert(entries, callback); * + * * //- - * // If you don't provide a callback, an EventEmitter is returned. Listen for - * // the error event to catch API and insert errors, and complete for when - * // the API request has completed. + * // If the callback is omitted, we'll return a Promise. + * //- + * table.insert(entries).then(function() { + * // All requested inserts have been processed. + * }); * //- - * table.insert(entries) - * .on('error', console.error) - * .on('complete', function() { - * // All requested inserts have been processed. - * }); */ Table.prototype.insert = function(entries, callback) { entries = arrify(entries).map(propAssign('method', Mutation.methods.INSERT)); @@ -846,15 +844,12 @@ Table.prototype.insert = function(entries, callback) { * table.mutate(entries, callback); * * //- - * // If you don't provide a callback, an EventEmitter is returned. Listen for - * // the error event to catch API and mutation errors, and complete for when - * // the API request has completed. + * // If the callback is omitted, we'll return a Promise. + * //- + * table.mutate(entries).then(function() { + * // All requested mutations have been processed. + * }); * //- - * table.mutate(entries) - * .on('error', console.error) - * .on('complete', function() { - * // All requested mutations have been processed. - * }); */ Table.prototype.mutate = function(entries, callback) { entries = flatten(arrify(entries)); @@ -870,13 +865,6 @@ Table.prototype.mutate = function(entries, callback) { entries: entries.map(Mutation.parse) }; - var isCallbackMode = is.function(callback); - var emitter = null; - - if (!isCallbackMode) { - emitter = new events.EventEmitter(); - } - var stream = pumpify.obj([ this.requestStream(grpcOpts, reqOpts), through.obj(function(data, enc, next) { @@ -889,13 +877,8 @@ Table.prototype.mutate = function(entries, callback) { } var status = common.GrpcService.decorateStatus_(entry.status); - status.entry = entries[entry.index]; - - if (!isCallbackMode) { - emitter.emit('error', status); - return; - } + status.entry = entries[entry.index]; throughStream.push(status); }); @@ -903,12 +886,6 @@ Table.prototype.mutate = function(entries, callback) { }) ]); - if (!isCallbackMode) { - stream.on('error', emitter.emit.bind(emitter, 'error')); - stream.on('finish', emitter.emit.bind(emitter, 'complete')); - return emitter; - } - stream .on('error', callback) .pipe(concat(function(mutationErrors) { @@ -1029,7 +1006,7 @@ Table.prototype.sampleRowKeysStream = function() { * that a callback is omitted. */ common.util.promisifyAll(Table, { - exclude: ['family', 'insert', 'mutate', 'row'] + exclude: ['family', 'row'] }); module.exports = Table; diff --git a/packages/bigtable/test/table.js b/packages/bigtable/test/table.js index b00e4b1a14c..c617dfb3e65 100644 --- a/packages/bigtable/test/table.js +++ b/packages/bigtable/test/table.js @@ -39,7 +39,7 @@ var fakeUtil = extend({}, common.util, { } promisified = true; - assert.deepEqual(options.exclude, ['family', 'insert', 'mutate', 'row']); + assert.deepEqual(options.exclude, ['family', 'row']); } }); @@ -859,17 +859,6 @@ describe('Bigtable/Table', function() { table.insert(fakeEntries, done); }); - - it('should return the mutate stream', function() { - var fakeStream = {}; - - table.mutate = function() { - return fakeStream; - }; - - var stream = table.insert([]); - assert.strictEqual(stream, fakeStream); - }); }); describe('mutate', function() { @@ -932,13 +921,6 @@ describe('Bigtable/Table', function() { done(); }); }); - - it('should emit the error via error event', function(done) { - table.mutate(entries).on('error', function(err) { - assert.strictEqual(err, error); - done(); - }); - }); }); describe('mutation errors', function() { @@ -993,69 +975,6 @@ describe('Bigtable/Table', function() { done(); }); }); - - it('should emit a mutation error as an error event', function(done) { - var mutationErrors = []; - var emitter = table.mutate(entries); - - assert(emitter instanceof events.EventEmitter); - - emitter - .on('error', function(err) { - mutationErrors.push(err); - }) - .on('complete', function() { - assert.strictEqual(mutationErrors[0], parsedStatuses[0]); - assert.strictEqual(mutationErrors[0].entry, entries[0]); - assert.strictEqual(mutationErrors[1], parsedStatuses[1]); - assert.strictEqual(mutationErrors[1].entry, entries[1]); - done(); - }); - }); - }); - }); - - describe('success', function() { - var fakeStatuses = [{ - index: 0, - status: { - code: 0 - } - }, { - index: 1, - status: { - code: 0 - } - }]; - - beforeEach(function() { - table.requestStream = function() { - var stream = through.obj(); - - stream.push({ entries: fakeStatuses }); - - setImmediate(function() { - stream.end(); - }); - - return stream; - }; - - FakeGrpcServiceObject.decorateStatus_ = function() { - throw new Error('Should not be called'); - }; - }); - - it('should emit the appropriate stream events', function(done) { - var emitter = table.mutate(entries); - - assert(emitter instanceof events.EventEmitter); - - emitter - .on('error', done) // should not be emitted - .on('complete', function() { - done(); - }); }); }); }); From 9ea569b68fd22b746aadb17e382a1465f4480d87 Mon Sep 17 00:00:00 2001 From: Dave Gramlich Date: Mon, 14 Nov 2016 17:02:03 -0500 Subject: [PATCH 2/4] linting --- packages/bigtable/src/table.js | 1 - packages/bigtable/test/table.js | 1 - 2 files changed, 2 deletions(-) diff --git a/packages/bigtable/src/table.js b/packages/bigtable/src/table.js index 8436d342699..48eee679461 100644 --- a/packages/bigtable/src/table.js +++ b/packages/bigtable/src/table.js @@ -23,7 +23,6 @@ var arrify = require('arrify'); var common = require('@google-cloud/common'); var concat = require('concat-stream'); -var events = require('events'); var flatten = require('lodash.flatten'); var is = require('is'); var propAssign = require('prop-assign'); diff --git a/packages/bigtable/test/table.js b/packages/bigtable/test/table.js index c617dfb3e65..29a6f249bd1 100644 --- a/packages/bigtable/test/table.js +++ b/packages/bigtable/test/table.js @@ -17,7 +17,6 @@ 'use strict'; var assert = require('assert'); -var events = require('events'); var extend = require('extend'); var nodeutil = require('util'); var proxyquire = require('proxyquire'); From 63e7c1a23156a05561fa2e9fc8cecb8a8f019ebf Mon Sep 17 00:00:00 2001 From: Dave Gramlich Date: Mon, 14 Nov 2016 17:22:15 -0500 Subject: [PATCH 3/4] reverted npm test script --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index b33586634f8..fcb9f1300b4 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "prepare-ghpages": "node ./scripts/docs/prepare.js", "remove-ghpages": "node ./scripts/docs/remove.js", "lint": "jshint scripts/ packages/ system-test/ test/ && jscs packages/ system-test/ test/", - "test": "rm -rf docs/json && npm run docs && npm run snippet-test && npm run unit-test", + "test": "npm run docs && npm run snippet-test && npm run unit-test", "unit-test": "mocha --timeout 5000 --bail packages/*/test/*.js", "snippet-test": "mocha --timeout 5000 --bail test/docs.js", "system-test": "mocha packages/*/system-test/*.js --no-timeouts --bail", From bf5e925c16239b49960b1b3717318a7caeaa0ae9 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Wed, 16 Nov 2016 12:15:01 -0500 Subject: [PATCH 4/4] simplifications & test coverage --- packages/bigtable/src/table.js | 33 +++++++++++++-------------------- packages/bigtable/test/table.js | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/packages/bigtable/src/table.js b/packages/bigtable/src/table.js index 48eee679461..d3de2f567d4 100644 --- a/packages/bigtable/src/table.js +++ b/packages/bigtable/src/table.js @@ -763,7 +763,7 @@ Table.prototype.insert = function(entries, callback) { * * @example * //- - * // Insert entities. See {module:bigtable/table#insert} + * // Insert entities. See {module:bigtable/table#insert}. * //- * var callback = function(err) { * if (err) { @@ -792,7 +792,7 @@ Table.prototype.insert = function(entries, callback) { * table.mutate(entries, callback); * * //- - * // Delete entities. See {module:bigtable/row#deleteCells} + * // Delete entities. See {module:bigtable/row#deleteCells}. * //- * var entries = [ * { @@ -848,7 +848,6 @@ Table.prototype.insert = function(entries, callback) { * table.mutate(entries).then(function() { * // All requested mutations have been processed. * }); - * //- */ Table.prototype.mutate = function(entries, callback) { entries = flatten(arrify(entries)); @@ -864,40 +863,34 @@ Table.prototype.mutate = function(entries, callback) { entries: entries.map(Mutation.parse) }; - var stream = pumpify.obj([ - this.requestStream(grpcOpts, reqOpts), - through.obj(function(data, enc, next) { - var throughStream = this; + var mutationErrors = []; - data.entries.forEach(function(entry) { - // mutation was successful, no need to notify the user + this.requestStream(grpcOpts, reqOpts) + .on('error', callback) + .on('data', function(obj) { + obj.entries.forEach(function(entry) { + // Mutation was successful. if (entry.status.code === 0) { return; } var status = common.GrpcService.decorateStatus_(entry.status); - status.entry = entries[entry.index]; - throughStream.push(status); - }); - next(); + mutationErrors.push(status); + }); }) - ]); - - stream - .on('error', callback) - .pipe(concat(function(mutationErrors) { + .on('end', function() { var err = null; - if (mutationErrors && mutationErrors.length > 0) { + if (mutationErrors.length > 0) { err = new common.util.PartialFailureError({ errors: mutationErrors }); } callback(err); - })); + }); }; /** diff --git a/packages/bigtable/test/table.js b/packages/bigtable/test/table.js index 29a6f249bd1..eb8b496f1c8 100644 --- a/packages/bigtable/test/table.js +++ b/packages/bigtable/test/table.js @@ -976,6 +976,39 @@ describe('Bigtable/Table', function() { }); }); }); + + describe('success', function() { + var fakeStatuses = [ + { + status: { + code: 0 + } + }, + { + status: { + code: 0 + } + } + ]; + + beforeEach(function() { + table.requestStream = function() { + var stream = new Stream({ + objectMode: true + }); + + setImmediate(function() { + stream.end({ entries: fakeStatuses }); + }); + + return stream; + }; + }); + + it('should execute callback', function(done) { + table.mutate(entries, done); + }); + }); }); describe('row', function() {