Issue 6488087: Add push notifications
|
Created:
12 years ago by Ali Afshar Modified:
9 years, 4 months ago Reviewers:
jcgregorio_google CC:
google-api-python-client_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixed from review comments. #
Total comments: 13
Patch Set 3 : Fix from review comments. #
Total comments: 4
Patch Set 4 : Fix from review comments. #
Total comments: 30
Patch Set 5 : Fix from review comments. #Patch Set 6 : Fix from review comments. #
Total comments: 3
Patch Set 7 : Update from review comments. #
Total comments: 1
Patch Set 8 : Update to latest push spec. #Patch Set 9 : Added warning note. #
Total comments: 5
Patch Set 10 : Fixed review comments. #Patch Set 11 : Added subscription ID support. #
MessagesTotal messages: 23
https://codereview.appspot.com/6488087/diff/1/apiclient/http.py File apiclient/http.py (right): https://codereview.appspot.com/6488087/diff/1/apiclient/http.py#newcode637 apiclient/http.py:637: def execute_with_headers(self, http=None): I don't like the idea of adding another method that is just execute() with slightly different semantics. Instead add two more methods to HttpRequest: def add_request_header(name, value) which adds another header to self.headers. Also add: def set_response_headers_callback(cb): The response_headers_callback is a callable that takes a single httplib2.Response object as an argument. Then in push.py add: def make_subscription(channel, request): """Returns (request, subscription)""" The request object can then be execute()'d or put into a batch request. https://codereview.appspot.com/6488087/diff/1/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/1/apiclient/push.py#newcode61 apiclient/push.py:61: def as_header(self): as_header_value https://codereview.appspot.com/6488087/diff/1/apiclient/push.py#newcode137 apiclient/push.py:137: class Subscription(object): Needs a verify() method that can be called, for example, from within the webhook request handler to verify the request against the client_token.
Sign in to reply to this message.
Sorry for the delay, this has been sitting around for a while. https://codereview.appspot.com/6488087/diff/1/apiclient/http.py File apiclient/http.py (right): https://codereview.appspot.com/6488087/diff/1/apiclient/http.py#newcode637 apiclient/http.py:637: def execute_with_headers(self, http=None): On 2012/09/06 13:34:44, jcgregorio_google wrote: > I don't like the idea of adding another method that is just execute() with > slightly different semantics. Instead add two more methods to HttpRequest: > > def add_request_header(name, value) > > which adds another header to self.headers. Also add: > > def set_response_headers_callback(cb): > > The response_headers_callback is a callable that takes a single > httplib2.Response object as an argument. > > Then in push.py add: > > def make_subscription(channel, request): > """Returns (request, subscription)""" > > The request object can then be execute()'d or put into a batch request. Done. https://codereview.appspot.com/6488087/diff/1/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/1/apiclient/push.py#newcode61 apiclient/push.py:61: def as_header(self): On 2012/09/06 13:34:44, jcgregorio_google wrote: > as_header_value Done. https://codereview.appspot.com/6488087/diff/1/apiclient/push.py#newcode137 apiclient/push.py:137: class Subscription(object): On 2012/09/06 13:34:44, jcgregorio_google wrote: > Needs a verify() method that can be called, for example, from within the webhook > request handler to verify the request against the client_token. Done.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/3001/apiclient/http.py File apiclient/http.py (right): https://codereview.appspot.com/6488087/diff/3001/apiclient/http.py#newcode48 apiclient/http.py:48: from push import Subscription Unused import. https://codereview.appspot.com/6488087/diff/3001/apiclient/http.py#newcode703 apiclient/http.py:703: def cb(headers): def cb(resp): # Where resp is an instance of httplib2.Response. https://codereview.appspot.com/6488087/diff/3001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/3001/apiclient/push.py#newcode152 apiclient/push.py:152: client_token: (optional) client token to verify the notification. Document return value, here and for for_channel(). https://codereview.appspot.com/6488087/diff/3001/apiclient/push.py#newcode185 apiclient/push.py:185: Returns: Blank line before Returns: https://codereview.appspot.com/6488087/diff/3001/apiclient/push.py#newcode190 apiclient/push.py:190: print headers, new_subscription.headers, self.headers rogue print https://codereview.appspot.com/6488087/diff/3001/tests/test_http.py File tests/test_http.py (right): https://codereview.appspot.com/6488087/diff/3001/tests/test_http.py#newcode48 tests/test_http.py:48: from apiclient.push import PushChannel PushChannel and Subscription are unused imports. https://codereview.appspot.com/6488087/diff/3001/tests/test_http.py#newcode811 tests/test_http.py:811: Needs unit tests for the headers callback.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/3001/apiclient/http.py File apiclient/http.py (right): https://codereview.appspot.com/6488087/diff/3001/apiclient/http.py#newcode48 apiclient/http.py:48: from push import Subscription On 2012/10/12 18:34:12, jcgregorio_google wrote: > Unused import. Done. https://codereview.appspot.com/6488087/diff/3001/apiclient/http.py#newcode703 apiclient/http.py:703: def cb(headers): On 2012/10/12 18:34:12, jcgregorio_google wrote: > def cb(resp): > # Where resp is an instance of httplib2.Response. Done. https://codereview.appspot.com/6488087/diff/3001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/3001/apiclient/push.py#newcode152 apiclient/push.py:152: client_token: (optional) client token to verify the notification. On 2012/10/12 18:34:12, jcgregorio_google wrote: > Document return value, here and for for_channel(). Done. https://codereview.appspot.com/6488087/diff/3001/apiclient/push.py#newcode185 apiclient/push.py:185: Returns: On 2012/10/12 18:34:12, jcgregorio_google wrote: > Blank line before Returns: Done. https://codereview.appspot.com/6488087/diff/3001/apiclient/push.py#newcode190 apiclient/push.py:190: print headers, new_subscription.headers, self.headers On 2012/10/12 18:34:12, jcgregorio_google wrote: > rogue print Done. https://codereview.appspot.com/6488087/diff/3001/tests/test_http.py File tests/test_http.py (right): https://codereview.appspot.com/6488087/diff/3001/tests/test_http.py#newcode48 tests/test_http.py:48: from apiclient.push import PushChannel On 2012/10/12 18:34:12, jcgregorio_google wrote: > PushChannel and Subscription are unused imports. Done.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/10001/apiclient/http.py File apiclient/http.py (right): https://codereview.appspot.com/6488087/diff/10001/apiclient/http.py#newcode645 apiclient/http.py:645: A tuple of (response headers, object model), where the object morel is a Comment needs to be reverted. https://codereview.appspot.com/6488087/diff/10001/apiclient/http.py#newcode658 apiclient/http.py:658: resp, body = self.next_chunk(http=http) Needs to be reverted.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/10001/apiclient/http.py File apiclient/http.py (right): https://codereview.appspot.com/6488087/diff/10001/apiclient/http.py#newcode645 apiclient/http.py:645: A tuple of (response headers, object model), where the object morel is a On 2012/10/13 18:43:40, jcgregorio_google wrote: > Comment needs to be reverted. Done. https://codereview.appspot.com/6488087/diff/10001/apiclient/http.py#newcode658 apiclient/http.py:658: resp, body = self.next_chunk(http=http) On 2012/10/13 18:43:40, jcgregorio_google wrote: > Needs to be reverted. Done.
Sign in to reply to this message.
Not sure if it sent this reply out. On 2012/10/13 19:07:00, Ali Afshar wrote: > https://codereview.appspot.com/6488087/diff/10001/apiclient/http.py > File apiclient/http.py (right): > > https://codereview.appspot.com/6488087/diff/10001/apiclient/http.py#newcode645 > apiclient/http.py:645: A tuple of (response headers, object model), where the > object morel is a > On 2012/10/13 18:43:40, jcgregorio_google wrote: > > Comment needs to be reverted. > > Done. > > https://codereview.appspot.com/6488087/diff/10001/apiclient/http.py#newcode658 > apiclient/http.py:658: resp, body = self.next_chunk(http=http) > On 2012/10/13 18:43:40, jcgregorio_google wrote: > > Needs to be reverted. > > Done.
Sign in to reply to this message.
Sorry for the slow review, still catching up from vacation. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode31 apiclient/push.py:31: class ClientTokenGenerator(object): Since there's no state, just make this a method, new_token(). https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode44 apiclient/push.py:44: class PushChannel(object): This is in push.py, so rename this to just Channel. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode55 apiclient/push.py:55: channel_name: str, the type of channel. if it's the type of channel why not name it channel_type? or is the description wrong? https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode86 apiclient/push.py:86: app_engine: bool, default=False, whether the notifications are to 'whether the destination for the notifications is an App Engine application.' https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode89 apiclient/push.py:89: PushChannel.__init__( Use super(). https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode99 apiclient/push.py:99: class PushHeaders(collections.defaultdict): Again, in push.py, so rename to just Headers(). https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode105 apiclient/push.py:105: Maybe overload __setitem__ and reject keys that aren't in ALL_HEADERS to save confusion. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode115 apiclient/push.py:115: yield header, self[header] yield header, value https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode160 apiclient/push.py:160: def _on_response_headers(response_headers, subscription=subscription): The callback gets the full response object, not just the headers, so make this _on_response(). Same rename for the first argument. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode168 apiclient/push.py:168: def for_channel(cls, channel, client_token=None): What use is there for a subscription that isn't associated with a request? https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode195 apiclient/push.py:195: new_subscription = Subscription() Why not just instantiate the PushHeaders object here and use that? https://codereview.appspot.com/6488087/diff/14001/tests/test_http.py File tests/test_http.py (right): https://codereview.appspot.com/6488087/diff/14001/tests/test_http.py#newcode810 tests/test_http.py:810: Also need test for add_request_header.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode31 apiclient/push.py:31: class ClientTokenGenerator(object): On 2012/10/23 15:30:17, jcgregorio_google wrote: > Since there's no state, just make this a method, new_token(). Done. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode44 apiclient/push.py:44: class PushChannel(object): On 2012/10/23 15:30:17, jcgregorio_google wrote: > This is in push.py, so rename this to just Channel. Done. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode55 apiclient/push.py:55: channel_name: str, the type of channel. On 2012/10/23 15:30:17, jcgregorio_google wrote: > if it's the type of channel why not name it channel_type? or is the description > wrong? Done. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode86 apiclient/push.py:86: app_engine: bool, default=False, whether the notifications are to On 2012/10/23 15:30:17, jcgregorio_google wrote: > 'whether the destination for the notifications is an App Engine application.' Done. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode89 apiclient/push.py:89: PushChannel.__init__( On 2012/10/23 15:30:17, jcgregorio_google wrote: > Use super(). Done. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode99 apiclient/push.py:99: class PushHeaders(collections.defaultdict): On 2012/10/23 15:30:17, jcgregorio_google wrote: > Again, in push.py, so rename to just Headers(). Done. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode105 apiclient/push.py:105: On 2012/10/23 15:30:17, jcgregorio_google wrote: > Maybe overload __setitem__ and reject keys that aren't in ALL_HEADERS to save > confusion. Not done. Will do if you want. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode115 apiclient/push.py:115: yield header, self[header] On 2012/10/23 15:30:17, jcgregorio_google wrote: > yield header, value Done. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode160 apiclient/push.py:160: def _on_response_headers(response_headers, subscription=subscription): On 2012/10/23 15:30:17, jcgregorio_google wrote: > The callback gets the full response object, not just the headers, so make this > _on_response(). Same rename for the first argument. Done. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode160 apiclient/push.py:160: def _on_response_headers(response_headers, subscription=subscription): On 2012/10/23 15:30:17, jcgregorio_google wrote: > The callback gets the full response object, not just the headers, so make this > _on_response(). Same rename for the first argument. Done. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode168 apiclient/push.py:168: def for_channel(cls, channel, client_token=None): On 2012/10/23 15:30:17, jcgregorio_google wrote: > What use is there for a subscription that isn't associated with a request? Subscription loaded from a notification eg webhook, or in future xmpp etc. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode195 apiclient/push.py:195: new_subscription = Subscription() On 2012/10/23 15:30:17, jcgregorio_google wrote: > Why not just instantiate the PushHeaders object here and use that? It would duplicate the code to read client tokens, i.e. one line :) https://codereview.appspot.com/6488087/diff/14001/tests/test_http.py File tests/test_http.py (right): https://codereview.appspot.com/6488087/diff/14001/tests/test_http.py#newcode810 tests/test_http.py:810: On 2012/10/23 15:30:17, jcgregorio_google wrote: > Also need test for add_request_header. Since push doesn't use that method, I removed it.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode105 apiclient/push.py:105: Yes, please do. On 2012/10/23 16:09:59, Ali Afshar wrote: > On 2012/10/23 15:30:17, jcgregorio_google wrote: > > Maybe overload __setitem__ and reject keys that aren't in ALL_HEADERS to save > > confusion. > > Not done. Will do if you want. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode168 apiclient/push.py:168: def for_channel(cls, channel, client_token=None): If that's the use case then does client_token=None make any sense? Wouldn't it always fail verification? On 2012/10/23 16:09:59, Ali Afshar wrote: > On 2012/10/23 15:30:17, jcgregorio_google wrote: > > What use is there for a subscription that isn't associated with a request? > > Subscription loaded from a notification eg webhook, or in future xmpp etc.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode105 apiclient/push.py:105: On 2012/10/25 16:26:01, jcgregorio_google wrote: > Yes, please do. > > On 2012/10/23 16:09:59, Ali Afshar wrote: > > On 2012/10/23 15:30:17, jcgregorio_google wrote: > > > Maybe overload __setitem__ and reject keys that aren't in ALL_HEADERS to > save > > > confusion. > > > > Not done. Will do if you want. > Done. https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode168 apiclient/push.py:168: def for_channel(cls, channel, client_token=None): On 2012/10/25 16:26:01, jcgregorio_google wrote: > If that's the use case then does client_token=None make any sense? Wouldn't it > always fail verification? > > On 2012/10/23 16:09:59, Ali Afshar wrote: > > On 2012/10/23 15:30:17, jcgregorio_google wrote: > > > What use is there for a subscription that isn't associated with a request? > > > > Subscription loaded from a notification eg webhook, or in future xmpp etc. > Sorry, I misanswered first time. This is just used by the other constructor. I think it is a reasonable architecture, decoupling the stateful part of being linked with a request and the pure part of creating the subscription. Your comment is correct though, this is useless for the purpose I stated.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/14001/apiclient/push.py#newcode168 apiclient/push.py:168: def for_channel(cls, channel, client_token=None): sgtm On 2012/10/25 17:50:15, Ali Afshar wrote: > On 2012/10/25 16:26:01, jcgregorio_google wrote: > > If that's the use case then does client_token=None make any sense? Wouldn't it > > always fail verification? > > > > On 2012/10/23 16:09:59, Ali Afshar wrote: > > > On 2012/10/23 15:30:17, jcgregorio_google wrote: > > > > What use is there for a subscription that isn't associated with a request? > > > > > > Subscription loaded from a notification eg webhook, or in future xmpp etc. > > > > Sorry, I misanswered first time. This is just used by the other constructor. I > think it is a reasonable architecture, decoupling the stateful part of being > linked with a request and the pure part of creating the subscription. > > Your comment is correct though, this is useless for the purpose I stated. https://codereview.appspot.com/6488087/diff/26001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/26001/apiclient/push.py#newcode114 apiclient/push.py:114: if key not in self.ALL_HEADERS: This is a case sensitive match?
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/26001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/26001/apiclient/push.py#newcode114 apiclient/push.py:114: if key not in self.ALL_HEADERS: On 2012/10/25 18:03:03, jcgregorio_google wrote: > This is a case sensitive match? Yes. Maybe I should fix the complete implementation to be case-insensitive.
Sign in to reply to this message.
On 2012/10/25 18:09:53, Ali Afshar wrote: Is this good to go?
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/26001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/26001/apiclient/push.py#newcode114 apiclient/push.py:114: if key not in self.ALL_HEADERS: Yes, make it case-insensitive. On 2012/10/25 18:09:30, Ali Afshar wrote: > On 2012/10/25 18:03:03, jcgregorio_google wrote: > > This is a case sensitive match? > > Yes. Maybe I should fix the complete implementation to be case-insensitive.
Sign in to reply to this message.
On 2012/11/05 19:10:47, jcgregorio_google wrote: > https://codereview.appspot.com/6488087/diff/26001/apiclient/push.py > File apiclient/push.py (right): > > https://codereview.appspot.com/6488087/diff/26001/apiclient/push.py#newcode114 > apiclient/push.py:114: if key not in self.ALL_HEADERS: > Yes, make it case-insensitive. > > On 2012/10/25 18:09:30, Ali Afshar wrote: > > On 2012/10/25 18:03:03, jcgregorio_google wrote: > > > This is a case sensitive match? > > > > Yes. Maybe I should fix the complete implementation to be case-insensitive. Done.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/35001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/35001/apiclient/push.py#newcode13 apiclient/push.py:13: """Push notifications support.""" I know you are updating this code now. While you are at it please add a note that the code here is experimental and subject to change.
Sign in to reply to this message.
On 2012/11/20 20:27:27, jcgregorio_google wrote: > https://codereview.appspot.com/6488087/diff/35001/apiclient/push.py > File apiclient/push.py (right): > > https://codereview.appspot.com/6488087/diff/35001/apiclient/push.py#newcode13 > apiclient/push.py:13: """Push notifications support.""" > I know you are updating this code now. While you are at it please add a note > that the code here is experimental and subject to change. Done.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/42001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/42001/apiclient/push.py#newcode34 apiclient/push.py:34: class UnsubscribableRequestError(ValueError): How about InvalidSubcriptionRequestError? https://codereview.appspot.com/6488087/diff/42001/apiclient/push.py#newcode196 apiclient/push.py:196: if request.method != 'GET': Add a unit test for this.
Sign in to reply to this message.
https://codereview.appspot.com/6488087/diff/42001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/42001/apiclient/push.py#newcode34 apiclient/push.py:34: class UnsubscribableRequestError(ValueError): On 2013/01/07 13:42:05, jcgregorio_google wrote: > How about InvalidSubcriptionRequestError? Done. https://codereview.appspot.com/6488087/diff/42001/apiclient/push.py#newcode196 apiclient/push.py:196: if request.method != 'GET': On 2013/01/07 13:42:05, jcgregorio_google wrote: > Add a unit test for this. There is already one: def test_non_get_error(self):
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6488087/diff/42001/apiclient/push.py File apiclient/push.py (right): https://codereview.appspot.com/6488087/diff/42001/apiclient/push.py#newcode196 apiclient/push.py:196: if request.method != 'GET': D'oh. Indeed there is. On 2013/01/07 15:18:04, Ali Afshar wrote: > On 2013/01/07 13:42:05, jcgregorio_google wrote: > > Add a unit test for this. > > There is already one: def test_non_get_error(self):
Sign in to reply to this message.
Committed in http://code.google.com/p/google-api-python-client/source/detail?r=30bef71e91a... On 2013/01/07 21:52:46, jcgregorio_google wrote: > LGTM > > https://codereview.appspot.com/6488087/diff/42001/apiclient/push.py > File apiclient/push.py (right): > > https://codereview.appspot.com/6488087/diff/42001/apiclient/push.py#newcode196 > apiclient/push.py:196: if request.method != 'GET': > D'oh. Indeed there is. > > On 2013/01/07 15:18:04, Ali Afshar wrote: > > On 2013/01/07 13:42:05, jcgregorio_google wrote: > > > Add a unit test for this. > > > > There is already one: def test_non_get_error(self):
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
