From 8200345a3570f0e56056875892f7e23c398c76a6 Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Thu, 28 Aug 2014 15:39:53 -0400 Subject: [PATCH 1/2] tests: connection refactor. --- lib/common/connection.js | 43 +++++++++++--------------- test/common/connection.js | 63 +++++++++++++++++---------------------- 2 files changed, 45 insertions(+), 61 deletions(-) diff --git a/lib/common/connection.js b/lib/common/connection.js index e7911521f9b..b1961e3dbc9 100644 --- a/lib/common/connection.js +++ b/lib/common/connection.js @@ -21,8 +21,10 @@ 'use strict'; +var events = require('events'); var fs = require('fs'); var GAPIToken = require('gapitoken'); +var nodeutil = require('util'); var req = require('request'); /** @type {module:common/util} */ @@ -91,6 +93,8 @@ module.exports.Token = Token; * var conn = new Connection({ scopes: SCOPES }); */ function Connection(opts) { + events.EventEmitter.call(this); + this.opts = opts || {}; this.credentials = null; @@ -98,7 +102,6 @@ function Connection(opts) { this.token = null; // existing access token, if exists this.isConnecting = false; - this.waitQueue = []; if (opts.credentials) { if (opts.credentials.client_email && opts.credentials.private_key) { @@ -110,18 +113,18 @@ function Connection(opts) { } } +nodeutil.inherits(Connection, events.EventEmitter); + /** * Retrieve a token to authorize requests. * * @todo Connect should be context aware, it should not require an email and * key, if it's running on Google Compute Engine. * - * @param {function} callback - The callback function. - * * @example - * conn.connect(function(err) {}); + * conn.connect(); */ -Connection.prototype.connect = function(callback) { +Connection.prototype.connect = function() { var that = this; this.isConnecting = true; // retrieves an access token @@ -130,7 +133,7 @@ Connection.prototype.connect = function(callback) { that.token = token; } that.isConnecting = false; - callback(err); + that.emit('connected'); }); }; @@ -246,30 +249,20 @@ Connection.prototype.createAuthorizedReq = function(reqOpts, callback) { reqOpts.headers['User-Agent'] = USER_AGENT; } - if (this.isConnected()) { - return callback(null, this.authorizeReq(reqOpts)); + function onConnected() { + callback(null, that.authorizeReq(reqOpts)); } - if (this.isConnecting) { - this.waitQueue = this.waitQueue || []; - this.waitQueue.push({ req: reqOpts, cb: callback }); + + if (this.isConnected()) { + onConnected(); return; } - this.connect(function(err) { - that.waitQueue.push({ req: reqOpts, cb: callback }); - that.waitQueue.forEach(function(v) { - if (!v.cb) { - return; - } - if (err) { - v.cb(err); - return; - } + this.on('connected', onConnected); - v.cb(null, that.authorizeReq(v.req)); - }); - that.waitQueue = []; - }); + if (!this.isConnecting) { + this.connect(); + } }; /** diff --git a/test/common/connection.js b/test/common/connection.js index 73e903982f8..6d4105e7a9f 100644 --- a/test/common/connection.js +++ b/test/common/connection.js @@ -32,6 +32,9 @@ describe('Connection', function() { conn = new connection.Connection({ keyFilename: path.join(__dirname, '../testdata/privateKeyFile.json') }); + conn.requester = function(opts, callback) { + callback(null); + }; }); it('should use a private key json file', function(done) { @@ -62,70 +65,58 @@ describe('Connection', function() { }); }); - describe('Token', function() { var tokenNeverExpires = new connection.Token('token', new Date(3000, 0, 0)); var tokenExpired = new connection.Token('token', new Date(2011, 0, 0)); it('should fetch a new token if token expires', function(done) { - var c = new connection.Connection({ - email: 'x@provider', - privateKey: '/some/path', - scopes: ['scope1', 'scope2'] - }); - c.token = tokenExpired; - c.fetchToken = function() { + conn.token = tokenExpired; + conn.fetchToken = function() { done(); }; - c.requester = function(opts, callback) { - callback(null); - }; - c.req({ uri: 'https://someuri' }, function() {}); + conn.req({ uri: 'https://someuri' }, function() {}); }); it('should make other requests wait while connecting', function(done) { var numTokenFetches = 0; - var c = new connection.Connection({ - email: 'x@provider', - privateKey: '/some/path', - scopes: ['scope1', 'scope2'] - }); - c.fetchToken = function(cb) { + var requestedUris = []; + conn.fetchToken = function(cb) { numTokenFetches++; setImmediate(function() { cb(null, tokenNeverExpires); }); }; - c.requester = function(opts, callback) { + conn.requester = function(opts, callback) { + requestedUris.push(opts.uri); callback(null); }; - async.parallel([ - function(done) { c.req({ uri: 'https://someuri' }, done); }, - function(done) { c.req({ uri: 'https://someuri' }, done); }, - function(done) { c.req({ uri: 'https://someuri' }, done); } + function(next) { + assert.strictEqual(conn.isConnecting, false); + conn.req({ uri: '1' }, next); + }, + function(next) { + assert.strictEqual(conn.isConnecting, true); + conn.req({ uri: '2' }, next); + }, + function(next) { + conn.req({ uri: '3' }, next); + } ], function(err) { - assert.equal(err, null); + assert.ifError(err); assert.equal(numTokenFetches, 1); - assert.equal(c.token, tokenNeverExpires); + assert.equal(conn.token, tokenNeverExpires); + assert.deepEqual(requestedUris, ['1', '2', '3']); done(); }); }); it('should fetch a new token if token is invalid', function(done) { - var c = new connection.Connection({ - email: 'x@provider', - privateKey: '/some/path', - scopes: ['scope1', 'scope2'] - }); - c.token = new connection.Token(); - c.fetchToken = function() { + conn.token = new connection.Token(); + conn.fetchToken = function() { done(); }; - c.requester = function(opts, callback) { - callback(null); - }; - c.req({ uri: 'https://someuri' }, function() {}); + conn.req({ uri: 'https://someuri' }, function() {}); }); }); }); From b91ad63561fb150427c80e6545f785906ebad4df Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Fri, 29 Aug 2014 16:24:44 -0400 Subject: [PATCH 2/2] add back error handling. --- lib/common/connection.js | 16 +++++++++++----- test/common/connection.js | 11 +++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/lib/common/connection.js b/lib/common/connection.js index b1961e3dbc9..4838d3233a0 100644 --- a/lib/common/connection.js +++ b/lib/common/connection.js @@ -129,10 +129,12 @@ Connection.prototype.connect = function() { this.isConnecting = true; // retrieves an access token this.fetchToken(function(err, token) { - if (!err) { - that.token = token; - } that.isConnecting = false; + if (err) { + that.emit('connected', err); + return; + } + that.token = token; that.emit('connected'); }); }; @@ -249,7 +251,11 @@ Connection.prototype.createAuthorizedReq = function(reqOpts, callback) { reqOpts.headers['User-Agent'] = USER_AGENT; } - function onConnected() { + function onConnected(err) { + if (err) { + callback(err); + return; + } callback(null, that.authorizeReq(reqOpts)); } @@ -258,7 +264,7 @@ Connection.prototype.createAuthorizedReq = function(reqOpts, callback) { return; } - this.on('connected', onConnected); + this.once('connected', onConnected); if (!this.isConnecting) { this.connect(); diff --git a/test/common/connection.js b/test/common/connection.js index 6d4105e7a9f..388a3304bdc 100644 --- a/test/common/connection.js +++ b/test/common/connection.js @@ -77,6 +77,17 @@ describe('Connection', function() { conn.req({ uri: 'https://someuri' }, function() {}); }); + it('should pass error to callback', function(done) { + var error = new Error('Something terrible happened.'); + conn.fetchToken = function(cb) { + cb(error); + }; + conn.req({}, function(err) { + assert.equal(error, err); + done(); + }); + }); + it('should make other requests wait while connecting', function(done) { var numTokenFetches = 0; var requestedUris = [];