From c43f35feff84b3869b60aa3ec77e0f683d73169b Mon Sep 17 00:00:00 2001 From: Stephen Sawchuk Date: Wed, 24 Jun 2015 10:20:35 -0400 Subject: [PATCH] pubsub: privatize createTopic + default autoCreate:true --- README.md | 6 +- lib/pubsub/index.js | 65 +++++++------- lib/pubsub/subscription.js | 6 +- lib/pubsub/topic.js | 91 ++++++++++---------- system-test/pubsub.js | 26 +++++- test/pubsub/index.js | 61 ++++++------- test/pubsub/topic.js | 170 +++++++++++++++++++++++++++++-------- 7 files changed, 268 insertions(+), 157 deletions(-) diff --git a/README.md b/README.md index 807787782542..5a5c09fe07a6 100644 --- a/README.md +++ b/README.md @@ -213,11 +213,7 @@ var pubsub = gcloud.pubsub({ keyFilename: '/path/to/keyfile.json' }); -// Create a new topic. -pubsub.createTopic('my-new-topic', function(err, topic) {}); - -// Reference an existing topic. -var topic = pubsub.topic('my-existing-topic'); +var topic = pubsub.topic('my-topic'); // Publish a message to the topic. topic.publish({ diff --git a/lib/pubsub/index.js b/lib/pubsub/index.js index 58a32c1f6884..f891eef46cec 100644 --- a/lib/pubsub/index.js +++ b/lib/pubsub/index.js @@ -148,34 +148,6 @@ PubSub.prototype.getTopics = function(query, callback) { }); }; -/** - * Create a topic with the given name. - * - * @param {string} name - Name of the topic. - * @param {function=} callback - The callback function. - * @param {?error} callback.err - An error from the API call, may be null. - * @param {module:pubsub/topic} callback.topic - The newly created topic. - * @param {object} callback.apiResponse - The full API response from the - * service. - * - * @example - * pubsub.createTopic('my-new-topic', function(err, topic, apiResponse) { - * topic.publish('New message!', function(err) {}); - * }); - */ -PubSub.prototype.createTopic = function(name, callback) { - callback = callback || util.noop; - var topic = this.topic(name); - var path = this.projectName + '/topics/' + name; - this.makeReq_('PUT', path, null, null, function(err, result) { - if (err) { - callback(err, null, result); - return; - } - callback(null, topic, result); - }); -}; - /** * Create a subscription to a topic. You may optionally provide an object to * customize the subscription. @@ -315,14 +287,13 @@ PubSub.prototype.subscription = function(name, options) { * * @param {string} name - The name of the topic. * @param {object=} options - Configuration object. - * @param {boolean=} options.autoCreate - Automatically create topic if it + * @param {boolean} options.autoCreate - Automatically create topic if it * doesn't exist. Note that messages published to a topic with no - * subscribers will not be delivered. + * subscribers will not be delivered. Default: true. * @return {module:pubsub/topic} * * @example - * var topic = pubsub.topic('my-existing-topic'); - * var topic = pubsub.topic('topic-that-maybe-exists', { autoCreate: true }); + * var topic = pubsub.topic('my-topic'); */ PubSub.prototype.topic = function(name, options) { if (!name) { @@ -436,6 +407,36 @@ PubSub.prototype.getSubscriptions = function(options, callback) { }); }; +/** + * Create a topic with the given name. + * + * @private + * + * @param {string} name - Name of the topic. + * @param {function=} callback - The callback function. + * @param {?error} callback.err - An error from the API call, may be null. + * @param {module:pubsub/topic} callback.topic - The newly created topic. + * @param {object} callback.apiResponse - The full API response from the + * service. + * + * @example + * pubsub.createTopic_('my-new-topic', function(err, topic, apiResponse) { + * topic.publish('New message!', function(err) {}); + * }); + */ +PubSub.prototype.createTopic_ = function(name, callback) { + callback = callback || util.noop; + var topic = this.topic(name); + var path = this.projectName + '/topics/' + name; + this.makeReq_('PUT', path, null, null, function(err, result) { + if (err) { + callback(err, null, result); + return; + } + callback(null, topic, result); + }); +}; + /** * Make a new request object from the provided arguments and wrap the callback * to intercept non-successful responses. diff --git a/lib/pubsub/subscription.js b/lib/pubsub/subscription.js index 5fa62ee8b0fc..87fc6ef8335c 100644 --- a/lib/pubsub/subscription.js +++ b/lib/pubsub/subscription.js @@ -76,7 +76,7 @@ var util = require('../common/util.js'); * //- * // From {@linkcode module:pubsub/topic#getSubscriptions}: * //- - * var topic = pubsub.topic('my-existing-topic'); + * var topic = pubsub.topic('my-topic'); * topic.getSubscriptions(function(err, subscriptions) { * // `subscriptions` is an array of Subscription objects. * }); @@ -84,7 +84,7 @@ var util = require('../common/util.js'); * //- * // From {@linkcode module:pubsub/topic#subscribe}: * //- - * var topic = pubsub.topic('my-existing-topic'); + * var topic = pubsub.topic('my-topic'); * topic.subscribe('new-subscription', function(err, subscription) { * // `subscription` is a Subscription object. * }); @@ -92,7 +92,7 @@ var util = require('../common/util.js'); * //- * // From {@linkcode module:pubsub/topic#subscription}: * //- - * var topic = pubsub.topic('my-existing-topic'); + * var topic = pubsub.topic('my-topic'); * var subscription = topic.subscription('my-existing-subscription'); * // `subscription` is a Subscription object. * diff --git a/lib/pubsub/topic.js b/lib/pubsub/topic.js index 87938b422b03..143939b17e46 100644 --- a/lib/pubsub/topic.js +++ b/lib/pubsub/topic.js @@ -30,12 +30,13 @@ var util = require('../common/util.js'); * * @param {module:pubsub} pubsub - PubSub object. * @param {object} options - Configuration object. + * @param {boolean=} options.autoCreate - Automatically create topic if it + * doesn't exist. Note that messages published to a topic with no + * subscribers will not be delivered. Default: true. * @param {string} options.name - Name of the topic. */ /** - * A Topic object allows you to interact with a Google Cloud Pub/Sub topic. To - * get this object, you will use the methods on the `pubsub` object, - * {@linkcode module:pubsub#topic} and {@linkcode module:pubsub#createTopic}. + * A Topic object allows you to interact with a Google Cloud Pub/Sub topic. * * @constructor * @alias module:pubsub/topic @@ -45,25 +46,15 @@ var util = require('../common/util.js'); * projectId: 'grape-spaceship-123' * }); * - * // From pubsub.topic: - * var topic = pubsub.topic('my-existing-topic'); - * - * // From pubsub.createTopic: - * pubsub.createTopic('my-new-topic', function(err, topic) { - * // `topic` is a Topic object. - * }); + * var topic = pubsub.topic('my-topic'); */ function Topic(pubsub, options) { - this.makeReq_ = pubsub.makeReq_.bind(pubsub); + this.autoCreate = options.autoCreate !== false; this.name = Topic.formatName_(pubsub.projectId, options.name); + this.projectId = pubsub.projectId; this.pubsub = pubsub; this.unformattedName = options.name; - - if (options.autoCreate) { - this.origMakeReq_ = this.makeReq_; - this.makeReq_ = this.autoCreateWrapper_; - } } /** @@ -99,34 +90,6 @@ Topic.formatName_ = function(projectId, name) { return 'projects/' + projectId + '/topics/' + name; }; -/** - * Wrapper for makeReq_ that automatically attempts to create a topic if it does - * not yet exist. - * - * @private - */ -Topic.prototype.autoCreateWrapper_ = function(method, path, q, body, callback) { - var self = this; - - function createAndRetry() { - self.pubsub.createTopic(self.unformattedName, function(err) { - if (err) { - callback(err); - return; - } - self.origMakeReq_(method, path, q, body, callback); - }); - } - - this.origMakeReq_(method, path, q, body, function(err, res) { - if (err && err.code === 404 && method !== 'DELETE') { - createAndRetry(); - } else { - callback(err, res); - } - }); -}; - /** * Publish the provided message or array of messages. On success, an array of * messageIds is returned in the response. @@ -144,7 +107,7 @@ Topic.prototype.autoCreateWrapper_ = function(method, path, q, body, callback) { * topic.publish({ * data: 'Hello, world!' * }, function(err, messageIds, apiResponse) {}); - * + * * //- * // The data property can be a JSON object as well. * //- @@ -159,7 +122,7 @@ Topic.prototype.autoCreateWrapper_ = function(method, path, q, body, callback) { * hello: 'world' * } * }; - * + * * topic.publish(registerMessage, function(err, messageIds, apiResponse) {}); * * //- @@ -321,4 +284,40 @@ Topic.prototype.subscription = function(name, options) { return this.pubsub.subscription(name, options); }; +/** + * Make an API request using the parent PubSub object's `makeReq_`. If the Topic + * instance has `autoCreate: true` set, this method will first try to create the + * Topic in the event of a 404. + * + * @private + * + * @param {string} method - Action. + * @param {string} path - Request path. + * @param {*} query - Request query object. + * @param {*} body - Request body contents. + * @param {function} callback - The callback function. + */ +Topic.prototype.makeReq_ = function(method, path, query, body, callback) { + var self = this; + + function createTopicThenRetryRequest() { + self.pubsub.createTopic_(self.unformattedName, function(err, topic, res) { + if (err) { + callback(err, null, res); + return; + } + + self.pubsub.makeReq_(method, path, query, body, callback); + }); + } + + this.pubsub.makeReq_(method, path, query, body, function(err, res) { + if (self.autoCreate && err && err.code === 404 && method !== 'DELETE') { + createTopicThenRetryRequest(); + } else { + callback(err, res); + } + }); +}; + module.exports = Topic; diff --git a/system-test/pubsub.js b/system-test/pubsub.js index 68b49a1df0dd..caf0dfe98b9e 100644 --- a/system-test/pubsub.js +++ b/system-test/pubsub.js @@ -51,7 +51,7 @@ describe('pubsub', function() { before(function(done) { // create all needed topics async.each(TOPIC_NAMES, function(name, cb) { - pubsub.createTopic(name, cb); + pubsub.createTopic_(name, cb); }, done); }); @@ -63,7 +63,6 @@ describe('pubsub', function() { }); describe('Topic', function() { - it('should be listed', function(done) { pubsub.getTopics(function(err, topics) { assert.ifError(err); @@ -92,12 +91,31 @@ describe('pubsub', function() { it('should be created and deleted', function(done) { var TOPIC_NAME = generateTopicName(); - pubsub.createTopic(TOPIC_NAME, function(err) { + pubsub.createTopic_(TOPIC_NAME, function(err) { assert.ifError(err); pubsub.topic(TOPIC_NAME).delete(done); }); }); + it('should lazily create by default', function(done) { + var newTopicName = generateTopicName(); + var newTopic = pubsub.topic(newTopicName); + + newTopic.publish({ data: 'message from me' }, function(err) { + assert.ifError(err); + + pubsub.getTopics(function(err, topics) { + assert.ifError(err); + + assert(topics.some(function(topic) { + return topic.name.indexOf(newTopicName) > -1; + })); + + newTopic.delete(done); + }); + }); + }); + it('should publish a message', function(done) { var topic = pubsub.topic(TOPIC_NAMES[0]); topic.publish({ data: 'message from me' }, function(err, messageIds) { @@ -131,7 +149,7 @@ describe('pubsub', function() { before(function(done) { // Create a new test topic. - pubsub.createTopic(TOPIC_NAME, function(err, newTopic) { + pubsub.createTopic_(TOPIC_NAME, function(err, newTopic) { assert.ifError(err); topic = newTopic; diff --git a/test/pubsub/index.js b/test/pubsub/index.js index 01527e39a787..b5066fdfdfec 100644 --- a/test/pubsub/index.js +++ b/test/pubsub/index.js @@ -143,36 +143,6 @@ describe('PubSub', function() { }); }); - describe('createTopic', function() { - it('should create a topic', function() { - var topicName = 'new-topic-name'; - pubsub.makeReq_ = function(method, path, q, body) { - assert.equal(method, 'PUT'); - assert.equal(path, 'projects/' + PROJECT_ID + '/topics/' + topicName); - assert.equal(body, null); - }; - pubsub.createTopic(topicName, function() {}); - }); - - it('should return a Topic object', function() { - pubsub.createTopic('new-topic', function(err, topic) { - assert.ifError(err); - assert(topic instanceof Topic); - }); - }); - - it('should pass apiResponse to callback', function(done) { - var resp = { success: true }; - pubsub.makeReq_ = function(method, path, q, body, callback) { - callback(null, resp); - }; - pubsub.createTopic('new-topic', function(err, topic, apiResponse) { - assert.equal(resp, apiResponse); - done(); - }); - }); - }); - describe('topic', function() { it('should throw if a name is not provided', function() { assert.throws(function() { @@ -502,6 +472,37 @@ describe('PubSub', function() { }); }); + describe('createTopic_', function() { + it('should create a topic', function() { + var topicName = 'new-topic-name'; + pubsub.makeReq_ = function(method, path, q, body) { + assert.equal(method, 'PUT'); + assert.equal(path, 'projects/' + PROJECT_ID + '/topics/' + topicName); + assert.equal(body, null); + }; + pubsub.createTopic_(topicName, function() {}); + }); + + it('should return a Topic object', function() { + pubsub.createTopic_('new-topic', function(err, topic) { + assert.ifError(err); + assert(topic instanceof Topic); + }); + }); + + it('should pass apiResponse to callback', function(done) { + var resp = { success: true }; + pubsub.makeReq_ = function(method, path, q, body, callback) { + callback(null, resp); + }; + pubsub.createTopic_('new-topic', function(err, topic, apiResponse) { + assert.equal(resp, apiResponse); + done(); + }); + }); + }); + + describe('makeReq_', function() { it('should pass network requests to the connection object', function(done) { var pubsub = new PubSub({ projectId: PROJECT_ID }); diff --git a/test/pubsub/topic.js b/test/pubsub/topic.js index 72cf6aa5b3bb..badf2b649e13 100644 --- a/test/pubsub/topic.js +++ b/test/pubsub/topic.js @@ -50,6 +50,18 @@ describe('Topic', function() { it('should assign pubsub object to `this`', function() { assert.deepEqual(topic.pubsub, pubsubMock); }); + + it('should set `autoCreate` to true by default', function() { + assert.strictEqual(topic.autoCreate, true); + }); + + it('should allow overriding autoCreate', function() { + var topic = new Topic(pubsubMock, { + name: TOPIC_NAME, + autoCreate: false + }); + assert.strictEqual(topic.autoCreate, false); + }); }); describe('formatMessage_', function() { @@ -145,43 +157,6 @@ describe('Topic', function() { }); }); - describe('publish to non-existing topic', function() { - var messageObject = { data: 'howdy' }; - - it('should generate 404 error without autoCreate', function(done) { - topic.makeReq_ = function(method, path, query, body, callback) { - callback({ code: 404 }); - }; - - topic.publish(messageObject, function(err) { - assert.equal(err.code, 404); - done(); - }); - }); - - it('should publish successfully with autoCreate', function(done) { - var acTopic = new Topic(pubsubMock, { - name: TOPIC_NAME, autoCreate: true - }); - var created = false; - - acTopic.origMakeReq_ = function(method, path, query, body, callback) { - if (!created) { - callback({ code: 404 }); - } else { - callback(null); - } - }; - - pubsubMock.createTopic = function(name, callback) { - created = true; - callback(); - }; - - acTopic.publish(messageObject, done); - }); - }); - describe('delete', function() { it('should delete a topic', function(done) { topic.makeReq_ = function(method, path) { @@ -275,4 +250,125 @@ describe('Topic', function() { doneFn(); }); }); + + describe('makeReq_', function() { + var method = 'POST'; + var path = '/path'; + var query = 'query'; + var body = 'body'; + + it('should call through to pubsub.makeReq_', function(done) { + topic.pubsub.makeReq_ = function(m, p, q, b) { + assert.equal(m, method); + assert.equal(p, path); + assert.equal(q, query); + assert.equal(b, body); + + done(); + }; + + topic.makeReq_(method, path, query, body, util.noop); + }); + + describe('autoCreate: false', function() { + it('should execute callback with response', function(done) { + var error = new Error('Error.'); + var apiResponse = { a: 'b', c: 'd' }; + + topic.pubsub.makeReq_ = function(method, path, query, body, callback) { + callback(error, apiResponse); + }; + + topic.makeReq_(method, path, query, body, function(err, apiResp) { + assert.deepEqual(err, error); + assert.deepEqual(apiResp, apiResponse); + + done(); + }); + }); + }); + + describe('autoCreate: true', function() { + it('should not create a topic if doing a DELETE', function(done) { + var topicCreated = false; + + topic.pubsub.createTopic_ = function() { + topicCreated = true; + }; + + topic.pubsub.makeReq_ = function(method, path, query, body, callback) { + callback({ code: 404 }); + }; + + topic.makeReq_('DELETE', path, query, body, function() { + assert.strictEqual(topicCreated, false); + + done(); + }); + }); + + it('should create a non-DELETE API request returns 404', function(done) { + topic.pubsub.createTopic_ = function(topicName) { + assert.equal(topicName, TOPIC_NAME); + + done(); + }; + + topic.pubsub.makeReq_ = function(method, path, query, body, callback) { + callback({ code: 404 }); + }; + + topic.makeReq_(method, path, query, body, util.noop); + }); + + describe('creating topic failed', function() { + var error = new Error('Error.'); + var apiResponse = { a: 'b', c: 'd' }; + + beforeEach(function() { + topic.pubsub.createTopic_ = function(topicName, callback) { + callback(error, null, apiResponse); + }; + + topic.pubsub.makeReq_ = function(m, p, q, b, callback) { + callback({ code: 404 }); + }; + }); + + it('should execute the callback with error & apiResp', function(done) { + topic.makeReq_(method, path, query, body, function(err, t, apiResp) { + assert.deepEqual(err, error); + assert.strictEqual(t, null); + assert.deepEqual(apiResp, apiResponse); + + done(); + }); + }); + }); + + describe('creating topic succeeded', function() { + it('should call makeReq_ again with the original args', function(done) { + topic.pubsub.createTopic_ = function(topicName, callback) { + callback(); + }; + + topic.pubsub.makeReq_ = function(m, p, q, b, callback) { + // Overwrite the method to confirm it is called again. + topic.pubsub.makeReq_ = function(m, p, q, b, callback) { + assert.equal(m, method); + assert.equal(p, path); + assert.equal(q, query); + assert.equal(b, body); + + callback(); // (should be the done function) + }; + + callback({ code: 404 }); + }; + + topic.makeReq_(method, path, query, body, done); + }); + }); + }); + }); });