Handle subscription list for specific topic separately. #1411

andrazhribernik · 2016-01-21T09:49:48Z

No description provided.

googlebot · 2016-01-21T09:49:50Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

andrazhribernik · 2016-01-21T10:03:06Z

Cytora signed it!

Update code

Handle subscription list for specific topic separately.

jgeewax · 2016-01-21T12:07:41Z

Thanks for the PR!

Not sure I understand what's happening here. Can you share a bit more about what you're trying to do? And maybe add some tests?

andrazhribernik · 2016-01-21T12:45:41Z

Sorry for not adding any description and tests. It is basically a bug fix.

When we tried to use this functionality:

from gcloud import pubsub
client = pubsub.Client(project=<project_name>)
client.list_subscriptions(topic_name=<topic_name>)

we got the following error.

TypeError                                 Traceback (most recent call last)
<ipython-input-3-8be845bada22> in <module>()
----> 1 client.list_subscriptions(topic_name="test_aw_2")

/Users/andrazhribernik/cytora/gcloud-python/gcloud/pubsub/client.py in list_subscriptions(self, page_size, page_token, topic_name)
    129         subscriptions = [Subscription.from_api_repr(resource, self,
    130                                                     topics=topics)
--> 131                          for resource in resp['subscriptions']]
    132         return subscriptions, resp.get('nextPageToken')
    133 

/Users/andrazhribernik/cytora/gcloud-python/gcloud/pubsub/subscription.pyc in from_api_repr(cls, resource, client, topics)
     66         if topics is None:
     67             topics = {}
---> 68         topic_path = resource['topic']
     69         topic = topics.get(topic_path)
     70         if topic is None:

TypeError: string indices must be integers

It also fails with different error if param topic_name is not specified

from gcloud import pubsub
client = pubsub.Client(project=<project_name>)
client.list_subscriptions()

returns

ValueError                                Traceback (most recent call last)
<ipython-input-4-39df795327d3> in <module>()
----> 1 client.list_subscriptions()

/Users/andrazhribernik/cytora/gcloud-python/gcloud/pubsub/client.py in list_subscriptions(self, page_size, page_token, topic_name)
    129         subscriptions = [Subscription.from_api_repr(resource, self,
    130                                                     topics=topics)
--> 131                          for resource in resp['subscriptions']]
    132         return subscriptions, resp.get('nextPageToken')
    133 

/Users/andrazhribernik/cytora/gcloud-python/gcloud/pubsub/subscription.pyc in from_api_repr(cls, resource, client, topics)
     71             # NOTE: This duplicates behavior from Topic.from_api_repr to avoid
     72             #       an import cycle.
---> 73             topic_name = topic_name_from_path(topic_path, client.project)
     74             topic = topics[topic_path] = client.topic(topic_name)
     75         _, _, _, name = resource['name'].split('/')

/Users/andrazhribernik/cytora/gcloud-python/gcloud/pubsub/_helpers.pyc in topic_name_from_path(path, project)
     36     if (len(path_parts) != 4 or path_parts[0] != 'projects' or
     37             path_parts[2] != 'topics'):
---> 38         raise ValueError('Expected path to be of the form '
     39                          'projects/{project}/topics/{topic_name}')
     40     if (len(path_parts) != 4 or path_parts[0] != 'projects' or

ValueError: Expected path to be of the form projects/{project}/topics/{topic_name}

The code we implemented basically solve a problem with inconsistency of GCloud REST API. It worked for us and was very quick fix. If you think this should be solved in a different way, I would be happy to hear your ideas.

I just took a look on tests now and it seems that tests do not reflect the output from GCloud REST API.

GCloud REST API response for a specific topic does not return a list of format [{'name': SUB_PATH, 'topic': TOPIC_PATH}], but returns list of strings (['topic1 name', 'topic2 name', ...]). It also seems pagination is on by default, which is not reflected in tests.

I am wondering. Is there a bug on GCLOUD REST API or on the gcloud-python side. If you think that this needs to be fix, I can refactor tests as well.

I hope it is clearer now. If you need any more informations, please do not hesitate to ask me.

jgeewax · 2016-01-21T15:42:17Z

/cc @tseaver : This seems like a legit concern. Curious why our system tests don't catch this?

setup.py

tseaver · 2016-01-28T18:59:38Z

@andrazhribernik can you supply the value of resource when the tracebacks are raised? I'm having a hard time following the logic for your changes.

andrazhribernik · 2016-01-31T15:35:27Z

@tseaver Value of resource is different whether param 'topic_name' is specified or not.

topic_name is not specified

from gcloud import pubsub
client = pubsub.Client(project=<project_name>)
client.list_subscriptions()

Error is raised, if there is any subscription for a topic that has already been deleted (_deleted-topic_).
Sample value of resp object:

{u'subscriptions': [
     {
        u'topic': u'projects/<project_id>/topics/<topic_name>',
        u'ackDeadlineSeconds': 10,
        u'pushConfig': {},
        u'name': u'projects//<project_id>/subscriptions//<subscription_name>''
    },
    {
        u'topic': u'_deleted-topic_',
        u'ackDeadlineSeconds': 10,
        u'pushConfig': {},
        u'name': u'projects/<project_id>/subscriptions/<subscription_name>'
    },

topic_name is specified

from gcloud import pubsub
client = pubsub.Client(project=<project_name>)
client.list_subscriptions(topic_name=<topic_name>)

Error is raised for every request of this type.
Sample value of resp object:

{u'subscriptions': [
        u'projects/<project_id>/subscriptions/<subscription_name1>',
        u'projects/<project_id>/subscriptions/<subscription_name2>'
    ]
}

I hope this explanation will help to understand my code updates.

googlebot · 2016-01-31T15:35:29Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

tseaver · 2016-02-15T18:45:10Z

@andrazhribernik I believe that #1440 should address this issue. @jgeewax, it also adds a system test for list_subscriptions (adding one for the list_topics case is too hard, as there is no guarantee that a given project will have no topics at all).

This allows generator users to set the numeric-enum-response parameter at generation time via the BUILD rule. The default is `False` for backward compatibility.

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Jan 21, 2016

Handle subscription list for specific topic separately.

3dc9cc0

Update code

andrazhribernik force-pushed the bug-fix-pubsub-list-subscriptions branch from c3030ba to 3dc9cc0 Compare January 21, 2016 10:20

Merge pull request #1 from cytora/bug-fix-pubsub-list-subscriptions

856a09e

Handle subscription list for specific topic separately.

Upgrade version in setup.py

08064bc

andrazhribernik force-pushed the bug-fix-pubsub-list-subscriptions branch from 6e7b4d3 to 08064bc Compare January 21, 2016 14:15

tseaver reviewed Jan 28, 2016
View reviewed changes

setup.py Outdated

This comment was marked as spam.

Sign in to view

Downgrade version.

4d016a4

tseaver closed this Feb 15, 2016

parthea pushed a commit that referenced this pull request Nov 26, 2025

chore: update token (#1411)

1988a60

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Handle subscription list for specific topic separately. #1411

Handle subscription list for specific topic separately. #1411

Uh oh!

andrazhribernik commented Jan 21, 2016

Uh oh!

googlebot commented Jan 21, 2016

Uh oh!

andrazhribernik commented Jan 21, 2016

Uh oh!

jgeewax commented Jan 21, 2016

Uh oh!

andrazhribernik commented Jan 21, 2016

Uh oh!

jgeewax commented Jan 21, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

tseaver commented Jan 28, 2016

Uh oh!

andrazhribernik commented Jan 31, 2016

Uh oh!

googlebot commented Jan 31, 2016

Uh oh!

tseaver commented Feb 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Handle subscription list for specific topic separately. #1411

Handle subscription list for specific topic separately. #1411

Uh oh!

Conversation

andrazhribernik commented Jan 21, 2016

Uh oh!

googlebot commented Jan 21, 2016

Uh oh!

andrazhribernik commented Jan 21, 2016

Uh oh!

jgeewax commented Jan 21, 2016

Uh oh!

andrazhribernik commented Jan 21, 2016

Uh oh!

jgeewax commented Jan 21, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

tseaver commented Jan 28, 2016

Uh oh!

andrazhribernik commented Jan 31, 2016

Uh oh!

googlebot commented Jan 31, 2016

Uh oh!

tseaver commented Feb 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants