From 1f80e368d45da5036e4f753d25f48a47fd758160 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 9 Jun 2015 10:36:46 -0700 Subject: [PATCH] Addressing review comments in PR #910. In particular: - Renaming `with_service_...` methods to `from_service_...` on `pubsub.Client`. - Implementing `topic.subscription` factory. - Using `topic.subscription` factory in docs and regression tests instead of instantiating a `Subscription`. - Updating docstring for `http` on `pubsub.Client` to describe the default behavior when no value is passed. - Making `client` a required / positional argument in `pubsub.Topic` (and ditching a test in the process). - Renaming `test_make_topic` as `test_topic`. --- docs/pubsub-usage.rst | 28 ++++++++++++++-------------- gcloud/pubsub/client.py | 8 +++++--- gcloud/pubsub/subscription.py | 4 +++- gcloud/pubsub/test_client.py | 10 +++++----- gcloud/pubsub/test_topic.py | 17 ++++++++++++++--- gcloud/pubsub/topic.py | 25 ++++++++++++++++++++++--- regression/pubsub.py | 7 +++---- 7 files changed, 66 insertions(+), 33 deletions(-) diff --git a/docs/pubsub-usage.rst b/docs/pubsub-usage.rst index 6ea045e0d3937..fd55f7607d1ae 100644 --- a/docs/pubsub-usage.rst +++ b/docs/pubsub-usage.rst @@ -12,9 +12,9 @@ Authorization / Configuration - The authentication credentials can be implicitly determined from the environment or directly via - :meth:`with_service_account_json ` + :meth:`from_service_account_json ` and - :meth:`with_service_account_p12 `. + :meth:`from_service_account_p12 `. - After setting ``GOOGLE_APPLICATION_CREDENTIALS`` and ``GCLOUD_PROJECT`` environment variables, create a :class:`Client ` @@ -121,7 +121,7 @@ Create a new pull subscription for a topic: >>> from gcloud import pubsub >>> client = pubsub.Client() >>> topic = client.topic('topic_name') - >>> subscription = pubsub.Subscription('subscription_name', topic) + >>> subscription = topic.subscription('subscription_name') >>> subscription.create() # API request Create a new pull subscription for a topic with a non-default ACK deadline: @@ -131,7 +131,7 @@ Create a new pull subscription for a topic with a non-default ACK deadline: >>> from gcloud import pubsub >>> client = pubsub.Client() >>> topic = client.topic('topic_name') - >>> subscription = pubsub.Subscription('subscription_name', ack_deadline=90) + >>> subscription = topic.subscription('subscription_name', ack_deadline=90) >>> subscription.create() # API request Create a new push subscription for a topic: @@ -142,8 +142,8 @@ Create a new push subscription for a topic: >>> ENDPOINT = 'https://example.com/hook' >>> client = pubsub.Client() >>> topic = client.topic('topic_name') - >>> subscription = pubsub.Subscription('subscription_name', - ... push_endpoint=ENDPOINT) + >>> subscription = topic.subscription('subscription_name', + ... push_endpoint=ENDPOINT) >>> subscription.create() # API request Check for the existence of a subscription: @@ -153,7 +153,7 @@ Check for the existence of a subscription: >>> from gcloud import pubsub >>> client = pubsub.Client() >>> topic = client.topic('topic_name') - >>> subscription = pubsub.Subscription('subscription_name', topic) + >>> subscription = topic.subscription('subscription_name') >>> subscription.exists() # API request True @@ -165,7 +165,7 @@ Convert a pull subscription to push: >>> ENDPOINT = 'https://example.com/hook' >>> client = pubsub.Client() >>> topic = client.topic('topic_name') - >>> subscription = pubsub.Subscription('subscription_name', topic) + >>> subscription = topic.subscription('subscription_name') >>> subscription.modify_push_configuration(push_endpoint=ENDPOINT) # API request Convert a push subscription to pull: @@ -176,8 +176,8 @@ Convert a push subscription to pull: >>> ENDPOINT = 'https://example.com/hook' >>> client = pubsub.Client() >>> topic = client.topic('topic_name') - >>> subscription = pubusb.Subscription('subscription_name', topic, - ... push_endpoint=ENDPOINT) + >>> subscription = topic.subscription('subscription_name', + ... push_endpoint=ENDPOINT) >>> subscription.modify_push_configuration(push_endpoint=None) # API request List subscriptions for a topic: @@ -208,7 +208,7 @@ Delete a subscription: >>> from gcloud import pubsub >>> client = pubsub.Client() >>> topic = client.topic('topic_name') - >>> subscription = pubsub.Subscription('subscription_name', topic) + >>> subscription = topic.subscription('subscription_name') >>> subscription.delete() # API request @@ -222,7 +222,7 @@ Fetch pending messages for a pull subscription: >>> from gcloud import pubsub >>> client = pubsub.Client() >>> topic = client.topic('topic_name') - >>> subscription = pubsub.Subscription('subscription_name', topic) + >>> subscription = topic.subscription('subscription_name') >>> with topic.batch() as batch: ... batch.publish('this is the first message_payload') ... batch.publish('this is the second message_payload', @@ -251,7 +251,7 @@ Fetch a limited number of pending messages for a pull subscription: >>> from gcloud import pubsub >>> client = pubsub.Client() >>> topic = client.topic('topic_name') - >>> subscription = pubsub.Subscription('subscription_name', topic) + >>> subscription = topic.subscription('subscription_name') >>> with topic.batch() as batch: ... batch.publish('this is the first message_payload') ... batch.publish('this is the second message_payload', @@ -267,7 +267,7 @@ Fetch messages for a pull subscription without blocking (none pending): >>> from gcloud import pubsub >>> client = pubsub.Client() >>> topic = client.topic('topic_name') - >>> subscription = pubsub.Subscription('subscription_name', topic) + >>> subscription = topic.subscription('subscription_name') >>> received = subscription.pull(max_messages=1) # API request >>> messages = [recv[1] for recv in received] >>> [message.id for message in messages] diff --git a/gcloud/pubsub/client.py b/gcloud/pubsub/client.py index 526937c16188c..cca81da176c6b 100644 --- a/gcloud/pubsub/client.py +++ b/gcloud/pubsub/client.py @@ -40,7 +40,9 @@ class Client(object): from the environment. :type http: :class:`httplib2.Http` or class that defines ``request()``. - :param http: An optional HTTP object to make requests. + :param http: An optional HTTP object to make requests. If not passed, an + ``http`` object is created that is bound to the + ``credentials`` for the current object. :raises: :class:`ValueError` if the project is neither passed in nor set in the environment. @@ -58,7 +60,7 @@ def __init__(self, project=None, credentials=None, http=None): self.connection = Connection(credentials=credentials, http=http) @classmethod - def with_service_account_json(cls, json_credentials_path, project=None): + def from_service_account_json(cls, json_credentials_path, project=None): """Factory to retrieve JSON credentials while creating client. :type json_credentials_path: string @@ -81,7 +83,7 @@ def with_service_account_json(cls, json_credentials_path, project=None): return cls(project=project, credentials=credentials) @classmethod - def with_service_account_p12(cls, client_email, private_key_path, + def from_service_account_p12(cls, client_email, private_key_path, project=None): """Factory to retrieve P12 credentials while creating client. diff --git a/gcloud/pubsub/subscription.py b/gcloud/pubsub/subscription.py index cb3023a286d77..fa0caffe31d2d 100644 --- a/gcloud/pubsub/subscription.py +++ b/gcloud/pubsub/subscription.py @@ -16,7 +16,6 @@ from gcloud.exceptions import NotFound from gcloud.pubsub.message import Message -from gcloud.pubsub.topic import Topic class Subscription(object): @@ -63,6 +62,9 @@ def from_api_repr(cls, resource, client, topics=None): :rtype: :class:`gcloud.pubsub.subscription.Subscription` :returns: Subscription parsed from ``resource``. """ + # pylint: disable=cyclic-import + from gcloud.pubsub.topic import Topic + # pylint: enable=cyclic-import if topics is None: topics = {} t_name = resource['topic'] diff --git a/gcloud/pubsub/test_client.py b/gcloud/pubsub/test_client.py index f2eb1ad257125..f0f1d30a81bed 100644 --- a/gcloud/pubsub/test_client.py +++ b/gcloud/pubsub/test_client.py @@ -82,7 +82,7 @@ def test_ctor_explicit(self): self.assertTrue(client_obj.connection._credentials is CREDS) self.assertEqual(CREDS._scopes, SCOPE) - def test_with_service_account_json(self): + def test_from_service_account_json(self): from gcloud._testing import _Monkey from gcloud.pubsub import client from gcloud.pubsub.connection import Connection @@ -98,7 +98,7 @@ def mock_creds(arg1): BOGUS_ARG = object() with _Monkey(client, get_for_service_account_json=mock_creds): - client_obj = KLASS.with_service_account_json( + client_obj = KLASS.from_service_account_json( BOGUS_ARG, project=PROJECT) self.assertEqual(client_obj.project, PROJECT) @@ -106,7 +106,7 @@ def mock_creds(arg1): self.assertTrue(client_obj.connection._credentials is CREDS) self.assertEqual(_CALLED, [(BOGUS_ARG,)]) - def test_with_service_account_p12(self): + def test_from_service_account_p12(self): from gcloud._testing import _Monkey from gcloud.pubsub import client from gcloud.pubsub.connection import Connection @@ -123,7 +123,7 @@ def mock_creds(arg1, arg2): BOGUS_ARG1 = object() BOGUS_ARG2 = object() with _Monkey(client, get_for_service_account_p12=mock_creds): - client_obj = KLASS.with_service_account_p12( + client_obj = KLASS.from_service_account_p12( BOGUS_ARG1, BOGUS_ARG2, project=PROJECT) self.assertEqual(client_obj.project, PROJECT) @@ -302,7 +302,7 @@ def test_list_subscriptions_with_topic_name(self): % (PROJECT, TOPIC_NAME)) self.assertEqual(req['query_params'], {}) - def test_make_topic(self): + def test_topic(self): PROJECT = 'PROJECT' TOPIC_NAME = 'TOPIC_NAME' CREDS = _Credentials() diff --git a/gcloud/pubsub/test_topic.py b/gcloud/pubsub/test_topic.py index 1f2c77fc150fe..b390104c26e2b 100644 --- a/gcloud/pubsub/test_topic.py +++ b/gcloud/pubsub/test_topic.py @@ -24,9 +24,6 @@ def _getTargetClass(self): def _makeOne(self, *args, **kw): return self._getTargetClass()(*args, **kw) - def test_ctor_wo_client(self): - self.assertRaises(ValueError, self._makeOne, 'TOPIC_NAME', client=None) - def test_ctor_w_explicit_timestamp(self): TOPIC_NAME = 'topic_name' PROJECT = 'PROJECT' @@ -334,6 +331,20 @@ def test_delete_w_alternate_client(self): self.assertEqual(req['method'], 'DELETE') self.assertEqual(req['path'], '/%s' % PATH) + def test_subscription(self): + from gcloud.pubsub.subscription import Subscription + TOPIC_NAME = 'topic_name' + PROJECT = 'PROJECT' + CLIENT = _Client(project=PROJECT) + topic = self._makeOne(TOPIC_NAME, + client=CLIENT) + + SUBSCRIPTION_NAME = 'subscription_name' + subscription = topic.subscription(SUBSCRIPTION_NAME) + self.assertTrue(isinstance(subscription, Subscription)) + self.assertEqual(subscription.name, SUBSCRIPTION_NAME) + self.assertTrue(subscription.topic is topic) + class TestBatch(unittest2.TestCase): diff --git a/gcloud/pubsub/topic.py b/gcloud/pubsub/topic.py index 9fe448e445735..41dcc2cd7be79 100644 --- a/gcloud/pubsub/topic.py +++ b/gcloud/pubsub/topic.py @@ -43,13 +43,32 @@ class Topic(object): to the attributes of each published message: the value will be an RFC 3339 timestamp. """ - def __init__(self, name, client=None, timestamp_messages=False): + def __init__(self, name, client, timestamp_messages=False): self.name = name - if client is None: - raise ValueError('Topic constructor requires a client.') self._client = client self.timestamp_messages = timestamp_messages + def subscription(self, name, ack_deadline=None, push_endpoint=None): + """Creates a subscription bound to the current topic. + + :type name: string + :param name: the name of the subscription + + :type ack_deadline: int + :param ack_deadline: the deadline (in seconds) by which messages pulled + from the back-end must be acknowledged. + + :type push_endpoint: string + :param push_endpoint: URL to which messages will be pushed by the + back-end. If not set, the application must pull + messages. + """ + # pylint: disable=cyclic-import + from gcloud.pubsub.subscription import Subscription + # pylint: enable=cyclic-import + return Subscription(name, self, ack_deadline=ack_deadline, + push_endpoint=push_endpoint) + @classmethod def from_api_repr(cls, resource, client): """Factory: construct a topic given its API representation diff --git a/regression/pubsub.py b/regression/pubsub.py index a3f83cf60d242..6b57921d31c8d 100644 --- a/regression/pubsub.py +++ b/regression/pubsub.py @@ -18,7 +18,6 @@ from gcloud import _helpers from gcloud import pubsub -from gcloud.pubsub.subscription import Subscription _helpers._PROJECT_ENV_VAR_NAME = 'GCLOUD_TESTS_PROJECT_ID' @@ -68,7 +67,7 @@ def test_create_subscription(self): topic.create() self.to_delete.append(topic) SUBSCRIPTION_NAME = 'subscribing-now' - subscription = Subscription(SUBSCRIPTION_NAME, topic) + subscription = topic.subscription(SUBSCRIPTION_NAME) self.assertFalse(subscription.exists()) subscription.create() self.to_delete.append(subscription) @@ -88,7 +87,7 @@ def test_list_subscriptions(self): 'newest%d' % (1000 * time.time(),), ] for subscription_name in subscriptions_to_create: - subscription = Subscription(subscription_name, topic) + subscription = topic.subscription(subscription_name) subscription.create() self.to_delete.append(subscription) @@ -106,7 +105,7 @@ def test_message_pull_mode_e2e(self): topic.create() self.to_delete.append(topic) SUBSCRIPTION_NAME = 'subscribing-now' - subscription = Subscription(SUBSCRIPTION_NAME, topic) + subscription = topic.subscription(SUBSCRIPTION_NAME) self.assertFalse(subscription.exists()) subscription.create() self.to_delete.append(subscription)