Issue 9885043: New push implementation sketch. (Closed)
|
Created:
11 years, 3 months ago by jcgregorio_google Modified:
11 years, 2 months ago CC:
google-api-python-client_googlegroups.com Visibility:
Public. |
DescriptionA new push implementation. This updates us to the latest revision of the push API which has a much simpler interface. Look in the module doc string for how this API would be used.
Patch Set 1 #Patch Set 2 : clean up code #Patch Set 3 : remove push.py #Patch Set 4 : working code #Patch Set 5 : Added tests. #
Total comments: 94
Patch Set 6 : comments #
Total comments: 15
Patch Set 7 : comments #Patch Set 8 : comments #Patch Set 9 : spacing #
MessagesTotal messages: 21
Sign in to reply to this message.
Is there no base file? Where did this live previously?
Sign in to reply to this message.
It lived in push.py previously, but basically every line of code changed. On Thu, May 30, 2013 at 3:01 PM, <dhermes@google.com> wrote: > Is there no base file? Where did this live previously? > > https://codereview.appspot.**com/9885043/<https://codereview.appspot.com/9885... > > -- > You received this message because you are subscribed to the Google Groups > "Google API Python Client" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to google-api-python-client+**unsubscribe@googlegroups.com<google-api-python-cli... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > >
Sign in to reply to this message.
Gotcher. I gave it a look, LMK when you want me to look again. On Thu, May 30, 2013 at 12:02 PM, Joe Gregorio <jcgregorio@google.com>wrote: > It lived in push.py previously, but basically every line of code changed. > > > On Thu, May 30, 2013 at 3:01 PM, <dhermes@google.com> wrote: > >> Is there no base file? Where did this live previously? >> >> https://codereview.appspot.**com/9885043/<https://codereview.appspot.com/9885... >> >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Google API Python Client" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to google-api-python-client+**unsubscribe@googlegroups.com<google-api-python-cli... >> . >> For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... >> . >> >> >> > -- > You received this message because you are subscribed to the Google Groups > "Google API Python Client" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to google-api-python-client+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > > > -- Danny Hermes Developer Programs Engineer
Sign in to reply to this message.
OK, cleaned up code and added unit tests. Also tested against a live API.
Sign in to reply to this message.
Overall comments: 1) Is there any client code where these are being used? 2) Why the need for this when a Channel object is specified in discovery docs? https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode8 apiclient/channel.py:8: - Notification does not dedup notification ids, that up to the receiver. Use "deduplicate" instead of "dedup". "that up to the receiver" should likely be "that's up..." https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode35 apiclient/channel.py:35: pass Instead of "pass" (here and the two below) add a comment, e.g. "# code to handle sync state". https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode36 apiclient/channel.py:36: elif n.resource_state == 'exists': Are these states unique to the Cloud Storage API or are they the same across all Discovery APIs which implement channels/notifications? Also, do they show up in Discovery? (I don't see them in https://www.googleapis.com/discovery/v1/apis/storage/v1beta2/rest) https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode55 apiclient/channel.py:55: EPOCH = datetime.datetime(1970, 1, 1) datetime.datetime.utcfromtimestamp(0) much less error prone/hardcoded badness https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode71 apiclient/channel.py:71: class Notification(): Inherit from object https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode83 apiclient/channel.py:83: Don't need this newline. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode92 apiclient/channel.py:92: of "exists", "not_exists", or "sync". Ditto my question above about these values https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:108: Only need one newline https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:111: type: str, The type of delivery mechanism used by this channel. Can you provide an example type? https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:131: You probably won't need to create this channel manually, since there are Who is you? How about "In user code, this Channel constructor will not typically be called manually, since..." https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:167: b = dict( http://doughellmann.com/2012/11/the-performance-impact-of-using-dict-instead-... Use {} as the constructor Also don't use "b" as a variable name. How about "json_body" or "result"? https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:188: such as the resource_id, which is needed when stopping an subscription. s/an subscription/a subscription https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:194: value = resp.get(json_name, None) Don't need the "None", resp.get(...) will use None as the default. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:196: setattr(self, param_name, value) Are there no attrs which should not be overwritten and/or cause an error if the value changes? e.g. it seems like "token" is in place for security and so changing that value would be no bueno. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:199: def notification_from_headers(channel, headers): Which style guide are we using here? https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:215: event_token = headers['X-GOOG-CHANNEL-TOKEN'] These headers should be globals. Also, if "headers" is a naive dict object, assuming the headers are all uppercase is problematic and error prone. For example, I've seen X-Goog-Channel-Token in places, but IIRC header case is not relevant. An easy solution (requiring no external libs) would be to change all the keys in "headers" to uppercase before doing a lookup or create a secondary mapping the all uppercase version to the header/headers in the original dict. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:223: message_number = int(headers['X-GOOG-MESSAGE-NUMBER']) What if this fails? https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:224: state = headers['X-GOOG-RESOURCE-STATE'] Ditto with putting these header values as globals. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:231: def new_webhook_channel(url, expiration=None, params=None): Ditto on style guide. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:247: d = expiration - EPOCH Use an expressive var name. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:250: return Channel("web_hook", uuid.uuid4().hex, Single or double quotes? (It is I, the human linter!)
Sign in to reply to this message.
Hi Joe, I finally had a chance to look at this. I added some comments. Cheers! David https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode16 apiclient/channel.py:16: channel.update(service.objects().watchAll( I think it would make it clearer if we split the two calls here: resp = service....watchAll(channel.body()).execute() channel.update(resp) https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode23 apiclient/channel.py:23: Example web_hook implementation. s/web_hook/webhook/ here and in all docstrings. The draft docs for Push are using "Webhook", it would be nice to be consistent. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode25 apiclient/channel.py:25: id = self.request.headers['X-GOOG-CHANNEL-ID'] comment below about case sensitive headers applies here too. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode77 apiclient/channel.py:77: Attributes: Why not all the headers in a notification? x-goog-changed x-goog-channel-id x-goog-channel-expiration x-goog-channel-token These things are useful for cases where the receiving app did not store the channel object. Not all use cases require that the receiving app know the channel details ahead of time. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode94 apiclient/channel.py:94: id: str, The unique identifier of the version of the resource at this "version" - Is the resource ID actually tied to the version (sort of like an etag)? the current wording in the draft Push doc for this is: "The ID of the watched resource. This ID is opaque and stable across API versions." https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:100: self.id = id Perhaps use resource_id instead of just id? not to be confused with channel id (which I originally assumed it was). Also, technically, this is not a "notification id" so I would find notification.id to be a misnomer. Same applies to uri and state I suppose. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:103: class Channel(object): Why is this class hand-written, instead of being created from the Discovery doc? https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:113: address: str, The address of the receiving entity where events are there are two spaces after str, https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:122: token: str, A unique value used to verify that notifications are The token is really for general purpose. Does not have to be unique. Verifying legit'ness is one example of what a token could be used for. Perhaps use the doc strings from the Channel schema in a Discovery doc: "An arbitrary string associated with the channel that is delivered to the target address with each event delivered over this channel." (except: s/event/notification/) https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:135: Args: same comments as for attributes above. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:141: expiration: int, The time, in milliseconds from the epoch, when this This is actually a string in the protocol. The Discovery schema looks like this: expiration: { type: "string", description: "The expiration instant for this channel if it is defined.", format: "int64" }, When this gets serialized to JSON, will it be "expiration": 12345, or "expiration": "12345" I think it must be the latter. Can you add a test case for this? https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:162: watchAll() methods as the value of body argument. The "All" in watchAll is actually a configurable suffix that each API can set. Perhaps just leave it as "... into watch() methods..." https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:191: resp: dict, The response from a watch() or watchAll() method. same thing about watchAll https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:199: def notification_from_headers(channel, headers): There is likely possibility that the first few notifications will arrive before the response to the watch() method (channel object won't be available in that case). How's the client app supposed to deal with that situation? Should we provide an example? provide other util methods to build a Notification object from just headers (no channel object)... make the channel object optional here? Perhaps change the flow a bit. Currently it looks like this: channel = new_webhook_channel() r = service....watch(channel).execute() channel.update(r) store.put(channel.id, channel) # on notification (which may actually arrive before store.put()!!!) channel = store.get(headers['x-goog-channel-id']) # may not be there n = notification_from_headers(channel, headers) Maybe change the notification side to something like: n = notification_from_headers(headers) try: channel = store.get(n.channel_id) except: # deal with fact that we got notification before we # stored channel object. n.validate(channel) # raises InvalidNotificationError And for use cases where receiving app does not need to have channel object, they just don't bother with store.put(), .get(), or n.validate() at all. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:206: from the web hook HTTP request. s/web hook/webhook/ https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:214: event_id = headers['X-GOOG-CHANNEL-ID'] s/event/channel/ here and other variables that use old 'event' names https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:215: event_token = headers['X-GOOG-CHANNEL-TOKEN'] On 2013/06/03 20:26:59, dhermes wrote: > These headers should be globals. Also, if "headers" is a naive dict object, > assuming the headers are all uppercase is problematic and error prone. For > example, I've seen X-Goog-Channel-Token in places, but IIRC header case is not > relevant. > > An easy solution (requiring no external libs) would be to change all the keys in > "headers" to uppercase before doing a lookup or create a secondary mapping the > all uppercase version to the header/headers in the original dict. +1 - http://stackoverflow.com/questions/5258977/are-http-headers-case-sensitive https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:215: event_token = headers['X-GOOG-CHANNEL-TOKEN'] The channel-token header may not be present (if no token was specified when the subscription was created)... this may raise a KeyError? https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:221: 'Channel token mismatch: %s !- %s' % (channel.token, event_token)) s/!-/!=/ ? https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:240: watch() or watchAll() method to see the value the service has set for ditto about watchAll https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:251: uuid.uuid4().hex, url, expiration=expiration_ms, I think it is strange to hardcode a UUID for the token. We should let the client optionally pass a token string as an arg to new_webkook_channel() https://codereview.appspot.com/9885043/diff/14001/tests/test_channel.py File tests/test_channel.py (right): https://codereview.appspot.com/9885043/diff/14001/tests/test_channel.py#newco... tests/test_channel.py:54: self.assertEqual(0, ch.expiration) Does this mean that if you call watch() with this, it would send a JSON body with an expiration of 0? That sounds like a problem :) https://codereview.appspot.com/9885043/diff/14001/tests/test_channel.py#newco... tests/test_channel.py:58: # New channel with an expiration time. I'm itching to see some other cases with expiration time eg: - before the epoch (should I get a negative number, or should it be truncated, should I get an exception?) - right at the epoch (so it results in 0) - a datetime with a timezone other than the one where this is running (or, since you can't control where this is running, two cases with two different timezones just to check that timezones are handled correctly) https://codereview.appspot.com/9885043/diff/14001/tests/test_channel.py#newco... tests/test_channel.py:88: 'X-GOOG-CHANNEL-ID': 'myid', add test cases with lower-case headers... and miXeD-Case? https://codereview.appspot.com/9885043/diff/14001/tests/test_channel.py#newco... tests/test_channel.py:89: 'X-GOOG-CHANNEL-TOKEN': 'mytoken', Add test cases where token header is not defined: token. Add test cases wehre expiration is defined.
Sign in to reply to this message.
https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode8 apiclient/channel.py:8: - Notification does not dedup notification ids, that up to the receiver. On 2013/06/03 20:26:59, dhermes wrote: > Use "deduplicate" instead of "dedup". "that up to the receiver" should likely be > "that's up..." Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode16 apiclient/channel.py:16: channel.update(service.objects().watchAll( On 2013/06/07 17:47:34, darist wrote: > I think it would make it clearer if we split the two calls here: > resp = service....watchAll(channel.body()).execute() > channel.update(resp) Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode23 apiclient/channel.py:23: Example web_hook implementation. On 2013/06/07 17:47:34, darist wrote: > s/web_hook/webhook/ here and in all docstrings. The draft docs for Push are > using "Webhook", it would be nice to be consistent. Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode25 apiclient/channel.py:25: id = self.request.headers['X-GOOG-CHANNEL-ID'] Skirted around the issue in the example by saying this is an example in webapp2. On 2013/06/07 17:47:34, darist wrote: > comment below about case sensitive headers applies here too. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode35 apiclient/channel.py:35: pass On 2013/06/03 20:26:59, dhermes wrote: > Instead of "pass" (here and the two below) add a comment, e.g. "# code to handle > sync state". Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode36 apiclient/channel.py:36: elif n.resource_state == 'exists': On 2013/06/03 20:26:59, dhermes wrote: > Are these states unique to the Cloud Storage API or are they the same across all > Discovery APIs which implement channels/notifications? Also, do they show up in > Discovery? (I don't see them in > https://www.googleapis.com/discovery/v1/apis/storage/v1beta2/rest) They are not in discovery. I'm not sure if that list is final, but they should be present across all APIs. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode55 apiclient/channel.py:55: EPOCH = datetime.datetime(1970, 1, 1) On 2013/06/03 20:26:59, dhermes wrote: > datetime.datetime.utcfromtimestamp(0) much less error prone/hardcoded badness Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode71 apiclient/channel.py:71: class Notification(): On 2013/06/03 20:26:59, dhermes wrote: > Inherit from object Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode77 apiclient/channel.py:77: Attributes: If they didn't store the channel then they can't confirm that the request is valid because they won't have the channel id to validate against. That's not a use case I want to enable. On 2013/06/07 17:47:34, darist wrote: > Why not all the headers in a notification? > x-goog-changed > x-goog-channel-id > x-goog-channel-expiration > x-goog-channel-token > > These things are useful for cases where the receiving app did not store the > channel object. Not all use cases require that the receiving app know the > channel details ahead of time. > https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode83 apiclient/channel.py:83: On 2013/06/03 20:26:59, dhermes wrote: > Don't need this newline. Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode94 apiclient/channel.py:94: id: str, The unique identifier of the version of the resource at this On 2013/06/07 17:47:34, darist wrote: > "version" - Is the resource ID actually tied to the version (sort of like an > etag)? > the current wording in the draft Push doc for this is: "The ID of the watched > resource. This ID is opaque and stable across API versions." Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:100: self.id = id On 2013/06/07 17:47:34, darist wrote: > Perhaps use resource_id instead of just id? not to be confused with channel id > (which I originally assumed it was). > > Also, technically, this is not a "notification id" so I would find > notification.id to be a misnomer. > > Same applies to uri and state I suppose. Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:103: class Channel(object): Python doesn't do model classes generated from discovery. On 2013/06/07 17:47:34, darist wrote: > Why is this class hand-written, instead of being created from the Discovery doc? https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:108: On 2013/06/03 20:26:59, dhermes wrote: > Only need one newline Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:111: type: str, The type of delivery mechanism used by this channel. On 2013/06/03 20:26:59, dhermes wrote: > Can you provide an example type? Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:113: address: str, The address of the receiving entity where events are On 2013/06/07 17:47:34, darist wrote: > there are two spaces after str, Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:122: token: str, A unique value used to verify that notifications are On 2013/06/07 17:47:34, darist wrote: > The token is really for general purpose. Does not have to be unique. Verifying > legit'ness is one example of what a token could be used for. Perhaps use the > doc strings from the Channel schema in a Discovery doc: "An arbitrary string > associated with the channel that is delivered to the target address with each > event delivered over this channel." (except: s/event/notification/) Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:131: You probably won't need to create this channel manually, since there are On 2013/06/03 20:26:59, dhermes wrote: > Who is you? How about > "In user code, this Channel constructor will not typically be called manually, > since..." Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:135: Args: On 2013/06/07 17:47:34, darist wrote: > same comments as for attributes above. Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:141: expiration: int, The time, in milliseconds from the epoch, when this On 2013/06/07 17:47:34, darist wrote: > This is actually a string in the protocol. > The Discovery schema looks like this: > expiration: { > type: "string", > description: "The expiration instant for this channel if it is defined.", > format: "int64" > }, > > When this gets serialized to JSON, will it be > "expiration": 12345, or > "expiration": "12345" > I think it must be the latter. Can you add a test case for this? The stack accepts the int, it only sends it as a string for the sake of Javascript. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:162: watchAll() methods as the value of body argument. On 2013/06/07 17:47:34, darist wrote: > The "All" in watchAll is actually a configurable suffix that each API can set. > Perhaps just leave it as "... into watch() methods..." Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:167: b = dict( On 2013/06/03 20:26:59, dhermes wrote: > http://doughellmann.com/2012/11/the-performance-impact-of-using-dict-instead-... > > Use {} as the constructor > > Also don't use "b" as a variable name. How about "json_body" or "result"? Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:188: such as the resource_id, which is needed when stopping an subscription. On 2013/06/03 20:26:59, dhermes wrote: > s/an subscription/a subscription Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:191: resp: dict, The response from a watch() or watchAll() method. On 2013/06/07 17:47:34, darist wrote: > same thing about watchAll Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:194: value = resp.get(json_name, None) On 2013/06/03 20:26:59, dhermes wrote: > Don't need the "None", resp.get(...) will use None as the default. Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:196: setattr(self, param_name, value) No, they could all be overwritten. On 2013/06/03 20:26:59, dhermes wrote: > Are there no attrs which should not be overwritten and/or cause an error if the > value changes? e.g. it seems like "token" is in place for security and so > changing that value would be no bueno. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:199: def notification_from_headers(channel, headers): On 2013/06/03 20:26:59, dhermes wrote: > Which style guide are we using here? The same one we always use: PEP8, but with 2 char indents. https://code.google.com/p/google-api-python-client/wiki/BecomingAContributor https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:199: def notification_from_headers(channel, headers): On 2013/06/07 17:47:34, darist wrote: > There is likely possibility that the first few notifications will arrive before > the response to the watch() method (channel object won't be available in that > case). > > How's the client app supposed to deal with that situation? Updated the sample in the comments at the top of the module to store the channel before calling watch(). > Should we provide an example? provide other util methods to build a Notification > object from just headers (no channel object)... make the channel object optional > here? > > Perhaps change the flow a bit. Currently it looks like this: > channel = new_webhook_channel() > r = service....watch(channel).execute() > channel.update(r) > store.put(channel.id, channel) > # on notification (which may actually arrive before store.put()!!!) > channel = store.get(headers['x-goog-channel-id']) # may not be there > n = notification_from_headers(channel, headers) > > Maybe change the notification side to something like: > n = notification_from_headers(headers) > try: > channel = store.get(n.channel_id) > except: > # deal with fact that we got notification before we > # stored channel object. > > n.validate(channel) # raises InvalidNotificationError > > And for use cases where receiving app does not need to have channel object, they > just don't bother with store.put(), .get(), or n.validate() at all. > > > https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:206: from the web hook HTTP request. On 2013/06/07 17:47:34, darist wrote: > s/web hook/webhook/ Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:215: event_token = headers['X-GOOG-CHANNEL-TOKEN'] Headers on the wire can be any case, but in most places they are uppercased, for example, by the CGI spec and webapp2. Sadly this isn't universal so yeah, I'm going to have to add defensive code here. You'd think in the year 2013 this would be a solved problem in Python. On 2013/06/03 20:26:59, dhermes wrote: > These headers should be globals. Also, if "headers" is a naive dict object, > assuming the headers are all uppercase is problematic and error prone. For > example, I've seen X-Goog-Channel-Token in places, but IIRC header case is not > relevant. > > An easy solution (requiring no external libs) would be to change all the keys in > "headers" to uppercase before doing a lookup or create a secondary mapping the > all uppercase version to the header/headers in the original dict. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:221: 'Channel token mismatch: %s !- %s' % (channel.token, event_token)) On 2013/06/07 17:47:34, darist wrote: > s/!-/!=/ ? Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:223: message_number = int(headers['X-GOOG-MESSAGE-NUMBER']) On 2013/06/03 20:26:59, dhermes wrote: > What if this fails? Then you are going to have a bad day. Added ValueError to the list of exceptions that can be raised. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:224: state = headers['X-GOOG-RESOURCE-STATE'] Added _upper_header_keys(). On 2013/06/03 20:26:59, dhermes wrote: > Ditto with putting these header values as globals. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:240: watch() or watchAll() method to see the value the service has set for On 2013/06/07 17:47:34, darist wrote: > ditto about watchAll Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:247: d = expiration - EPOCH On 2013/06/03 20:26:59, dhermes wrote: > Use an expressive var name. Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:250: return Channel("web_hook", uuid.uuid4().hex, On 2013/06/03 20:26:59, dhermes wrote: > Single or double quotes? (It is I, the human linter!) Done. https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:251: uuid.uuid4().hex, url, expiration=expiration_ms, On 2013/06/07 17:47:34, darist wrote: > I think it is strange to hardcode a UUID for the token. We should let the client > optionally pass a token string as an arg to new_webkook_channel() Done. https://codereview.appspot.com/9885043/diff/14001/tests/test_channel.py File tests/test_channel.py (right): https://codereview.appspot.com/9885043/diff/14001/tests/test_channel.py#newco... tests/test_channel.py:54: self.assertEqual(0, ch.expiration) No, see the test on line 25. On 2013/06/07 17:47:34, darist wrote: > Does this mean that if you call watch() with this, it would send a JSON body > with an expiration of 0? That sounds like a problem :) https://codereview.appspot.com/9885043/diff/14001/tests/test_channel.py#newco... tests/test_channel.py:58: # New channel with an expiration time. On 2013/06/07 17:47:34, darist wrote: > I'm itching to see some other cases with expiration time > eg: > - before the epoch (should I get a negative number, or should it be truncated, > should I get an exception?) > - right at the epoch (so it results in 0) Values of 0 or less are ignored. > - a datetime with a timezone other than the one where this is running (or, since > you can't control where this is running, two cases with two different timezones > just to check that timezones are handled correctly) I'm not going to test the standard library datetime module. https://codereview.appspot.com/9885043/diff/14001/tests/test_channel.py#newco... tests/test_channel.py:88: 'X-GOOG-CHANNEL-ID': 'myid', On 2013/06/07 17:47:34, darist wrote: > add test cases with lower-case headers... and miXeD-Case? Done. https://codereview.appspot.com/9885043/diff/14001/tests/test_channel.py#newco... tests/test_channel.py:89: 'X-GOOG-CHANNEL-TOKEN': 'mytoken', Dropped token entirely since it's not used. On 2013/06/07 17:47:34, darist wrote: > Add test cases where token header is not defined: token. > > Add test cases wehre expiration is defined.
Sign in to reply to this message.
https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcode25 apiclient/channel.py:25: channel.update(resp) Should the channel be stored again after update()? the copy that was stored before calling watchAll() is different (eg, didn't have resource_id) https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:234: if channel.id != channel_id: Now that you are not validating the token here, should we add the token to the Notification object? So they user can validate it against their stored channel.token if they wish to? https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:271: return Channel('web_hook', uuid.uuid4().hex, It does not have to use the hex format. Draft docs and examples are using the alphanumeric format, eg: 550e8400-e29b-41d4-a716-446655440000
Sign in to reply to this message.
https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:103: class Channel(object): Yet :) On 2013/06/13 15:04:18, jcgregorio_google wrote: > Python doesn't do model classes generated from discovery. > > On 2013/06/07 17:47:34, darist wrote: > > Why is this class hand-written, instead of being created from the Discovery > doc? > https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:196: setattr(self, param_name, value) So you could send your watch request with a "token" which you use for security reasons and then watch sends you back a different value, so the one you're using for security is no longer used? On 2013/06/13 15:04:18, jcgregorio_google wrote: > No, they could all be overwritten. > > On 2013/06/03 20:26:59, dhermes wrote: > > Are there no attrs which should not be overwritten and/or cause an error if > the > > value changes? e.g. it seems like "token" is in place for security and so > > changing that value would be no bueno. > https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:215: event_token = headers['X-GOOG-CHANNEL-TOKEN'] You still haven't made these globals. Could you do that? On 2013/06/13 15:04:18, jcgregorio_google wrote: > Headers on the wire can be any case, but in most places they are uppercased, for > example, by the CGI spec and webapp2. Sadly this isn't universal so yeah, I'm > going to have to add defensive code here. You'd think in the year 2013 this > would be a solved problem in Python. > > On 2013/06/03 20:26:59, dhermes wrote: > > These headers should be globals. Also, if "headers" is a naive dict object, > > assuming the headers are all uppercase is problematic and error prone. For > > example, I've seen X-Goog-Channel-Token in places, but IIRC header case is not > > relevant. > > > > An easy solution (requiring no external libs) would be to change all the keys > in > > "headers" to uppercase before doing a lookup or create a secondary mapping the > > all uppercase version to the header/headers in the original dict. > https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:251: uuid.uuid4().hex, url, expiration=expiration_ms, Should we assign token = token or uuid.uuid4().hex for the case where no value is passed in? On 2013/06/13 15:04:18, jcgregorio_google wrote: > On 2013/06/07 17:47:34, darist wrote: > > I think it is strange to hardcode a UUID for the token. We should let the > client > > optionally pass a token string as an arg to new_webkook_channel() > > Done. https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcode95 apiclient/channel.py:95: resource_id: str, The unique identifier of the version of the resource at this line length https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:123: example, 'web_hook'. Didn't darist say to use webhook instead of web_hook? https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:150: example, 'web_hook'. Ditto web_hook -> webhook https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:271: return Channel('web_hook', uuid.uuid4().hex, web_hook -> webhook
Sign in to reply to this message.
https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:251: uuid.uuid4().hex, url, expiration=expiration_ms, On 2013/06/13 16:14:55, dhermes wrote: > Should we assign token = token or uuid.uuid4().hex for the case where no value > is passed in? > I think token = token is good. If user does not want to provide a token, no reason to force our own. > On 2013/06/13 15:04:18, jcgregorio_google wrote: > > On 2013/06/07 17:47:34, darist wrote: > > > I think it is strange to hardcode a UUID for the token. We should let the > > client > > > optionally pass a token string as an arg to new_webkook_channel() > > > > Done. > https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:123: example, 'web_hook'. On 2013/06/13 16:14:55, dhermes wrote: > Didn't darist say to use webhook instead of web_hook? I did... We are using the term "Webhook" in documentation prose, but the type value remains 'type': 'web_hook' for now. https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:150: example, 'web_hook'. On 2013/06/13 16:14:55, dhermes wrote: > Ditto web_hook -> webhook should remain web_hook for now https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:271: return Channel('web_hook', uuid.uuid4().hex, On 2013/06/13 16:14:55, dhermes wrote: > web_hook -> webhook ditto
Sign in to reply to this message.
https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:123: example, 'web_hook'. Ha! This is very confusing, for what gain? On 2013/06/13 18:30:04, darist wrote: > On 2013/06/13 16:14:55, dhermes wrote: > > Didn't darist say to use webhook instead of web_hook? > I did... We are using the term "Webhook" in documentation prose, but the type > value remains 'type': 'web_hook' for now.
Sign in to reply to this message.
https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcode25 apiclient/channel.py:25: channel.update(resp) So notifications for a resource_id could come in before the watchAll() call returns, which means even if the channel gets stored, the resource_id might not be valid when the notification arrives? So is the only use of the resource_id for stopping a subscription? I've updated the sample to add storing the channel again after watchAll returns. On 2013/06/13 16:13:27, darist wrote: > Should the channel be stored again after update()? the copy that was stored > before calling watchAll() is different (eg, didn't have resource_id) https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcode95 apiclient/channel.py:95: resource_id: str, The unique identifier of the version of the resource at this On 2013/06/13 16:14:55, dhermes wrote: > line length Done. https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:234: if channel.id != channel_id: No, because it hasn't changed, right? Isn't it as invariant as the id? If so then if the user wants the token value they can get it from the Channel. On 2013/06/13 16:13:27, darist wrote: > Now that you are not validating the token here, should we add the token to the > Notification object? So they user can validate it against their stored > channel.token if they wish to? https://codereview.appspot.com/9885043/diff/30001/apiclient/channel.py#newcod... apiclient/channel.py:271: return Channel('web_hook', uuid.uuid4().hex, On 2013/06/13 16:13:27, darist wrote: > It does not have to use the hex format. Draft docs and examples are using the > alphanumeric format, eg: 550e8400-e29b-41d4-a716-446655440000 Done.
Sign in to reply to this message.
ping
Sign in to reply to this message.
On 2013/06/24 18:00:11, jcgregorio_google wrote: > ping Still have some unresolved comments. In particular "So you could send your watch request with a "token" which you use for security reasons and then watch sends you back a different value, so the one you're using for security is no longer used?" and "You still haven't made these globals. Could you do that?" in https://codereview.appspot.com/9885043/diff2/14001:30001/apiclient/channel.py
Sign in to reply to this message.
https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py File apiclient/channel.py (right): https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:196: setattr(self, param_name, value) Per the previous discussion, the id uuid is supposed to be used for security, not the token. On 2013/06/13 16:14:55, dhermes wrote: > So you could send your watch request with a "token" which you use for security > reasons and then watch sends you back a different value, so the one you're using > for security is no longer used? > > On 2013/06/13 15:04:18, jcgregorio_google wrote: > > No, they could all be overwritten. > > > > On 2013/06/03 20:26:59, dhermes wrote: > > > Are there no attrs which should not be overwritten and/or cause an error if > > the > > > value changes? e.g. it seems like "token" is in place for security and so > > > changing that value would be no bueno. > > > https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcod... apiclient/channel.py:215: event_token = headers['X-GOOG-CHANNEL-TOKEN'] On 2013/06/13 16:14:55, dhermes wrote: > You still haven't made these globals. Could you do that? > > On 2013/06/13 15:04:18, jcgregorio_google wrote: > > Headers on the wire can be any case, but in most places they are uppercased, > for > > example, by the CGI spec and webapp2. Sadly this isn't universal so yeah, I'm > > going to have to add defensive code here. You'd think in the year 2013 this > > would be a solved problem in Python. > > > > On 2013/06/03 20:26:59, dhermes wrote: > > > These headers should be globals. Also, if "headers" is a naive dict object, > > > assuming the headers are all uppercase is problematic and error prone. For > > > example, I've seen X-Goog-Channel-Token in places, but IIRC header case is > not > > > relevant. > > > > > > An easy solution (requiring no external libs) would be to change all the > keys > > in > > > "headers" to uppercase before doing a lookup or create a secondary mapping > the > > > all uppercase version to the header/headers in the original dict. > > > Done.
Sign in to reply to this message.
So what is the point of the token? I guess I missed that discussion. On Tue, Jun 25, 2013 at 10:48 AM, <jcgregorio@google.com> wrote: > > https://codereview.appspot.**com/9885043/diff/14001/**apiclient/channel.py<ht... > File apiclient/channel.py (right): > > https://codereview.appspot.**com/9885043/diff/14001/** > apiclient/channel.py#**newcode196<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode196> > apiclient/channel.py:196: setattr(self, param_name, value) > Per the previous discussion, the id uuid is supposed to be used for > security, not the token. > > > On 2013/06/13 16:14:55, dhermes wrote: > >> So you could send your watch request with a "token" which you use for >> > security > >> reasons and then watch sends you back a different value, so the one >> > you're using > >> for security is no longer used? >> > > On 2013/06/13 15:04:18, jcgregorio_google wrote: >> > No, they could all be overwritten. >> > >> > On 2013/06/03 20:26:59, dhermes wrote: >> > > Are there no attrs which should not be overwritten and/or cause an >> > error if > >> > the >> > > value changes? e.g. it seems like "token" is in place for security >> > and so > >> > > changing that value would be no bueno. >> > >> > > > https://codereview.appspot.**com/9885043/diff/14001/** > apiclient/channel.py#**newcode215<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode215> > apiclient/channel.py:215: event_token = headers['X-GOOG-CHANNEL-TOKEN'**] > On 2013/06/13 16:14:55, dhermes wrote: > >> You still haven't made these globals. Could you do that? >> > > On 2013/06/13 15:04:18, jcgregorio_google wrote: >> > Headers on the wire can be any case, but in most places they are >> > uppercased, > >> for >> > example, by the CGI spec and webapp2. Sadly this isn't universal so >> > yeah, I'm > >> > going to have to add defensive code here. You'd think in the year >> > 2013 this > >> > would be a solved problem in Python. >> > >> > On 2013/06/03 20:26:59, dhermes wrote: >> > > These headers should be globals. Also, if "headers" is a naive >> > dict object, > >> > > assuming the headers are all uppercase is problematic and error >> > prone. For > >> > > example, I've seen X-Goog-Channel-Token in places, but IIRC header >> > case is > >> not >> > > relevant. >> > > >> > > An easy solution (requiring no external libs) would be to change >> > all the > >> keys >> > in >> > > "headers" to uppercase before doing a lookup or create a secondary >> > mapping > >> the >> > > all uppercase version to the header/headers in the original dict. >> > >> > > > Done. > > https://codereview.appspot.**com/9885043/<https://codereview.appspot.com/9885... > -- Danny Hermes Developer Programs Engineer
Sign in to reply to this message.
Yeah, at one point it was supposed to be used for verification, now it's just a place to store extra info that gets passed back to the caller. But, I will point out, I don't disagree, it doesn't make much sense. On Tue, Jun 25, 2013 at 3:16 PM, Danny Hermes <dhermes@google.com> wrote: > So what is the point of the token? I guess I missed that discussion. > > > On Tue, Jun 25, 2013 at 10:48 AM, <jcgregorio@google.com> wrote: > >> >> https://codereview.appspot.**com/9885043/diff/14001/** >> apiclient/channel.py<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py> >> File apiclient/channel.py (right): >> >> https://codereview.appspot.**com/9885043/diff/14001/** >> apiclient/channel.py#**newcode196<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode196> >> apiclient/channel.py:196: setattr(self, param_name, value) >> Per the previous discussion, the id uuid is supposed to be used for >> security, not the token. >> >> >> On 2013/06/13 16:14:55, dhermes wrote: >> >>> So you could send your watch request with a "token" which you use for >>> >> security >> >>> reasons and then watch sends you back a different value, so the one >>> >> you're using >> >>> for security is no longer used? >>> >> >> On 2013/06/13 15:04:18, jcgregorio_google wrote: >>> > No, they could all be overwritten. >>> > >>> > On 2013/06/03 20:26:59, dhermes wrote: >>> > > Are there no attrs which should not be overwritten and/or cause an >>> >> error if >> >>> > the >>> > > value changes? e.g. it seems like "token" is in place for security >>> >> and so >> >>> > > changing that value would be no bueno. >>> > >>> >> >> >> https://codereview.appspot.**com/9885043/diff/14001/** >> apiclient/channel.py#**newcode215<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode215> >> apiclient/channel.py:215: event_token = headers['X-GOOG-CHANNEL-TOKEN'**] >> On 2013/06/13 16:14:55, dhermes wrote: >> >>> You still haven't made these globals. Could you do that? >>> >> >> On 2013/06/13 15:04:18, jcgregorio_google wrote: >>> > Headers on the wire can be any case, but in most places they are >>> >> uppercased, >> >>> for >>> > example, by the CGI spec and webapp2. Sadly this isn't universal so >>> >> yeah, I'm >> >>> > going to have to add defensive code here. You'd think in the year >>> >> 2013 this >> >>> > would be a solved problem in Python. >>> > >>> > On 2013/06/03 20:26:59, dhermes wrote: >>> > > These headers should be globals. Also, if "headers" is a naive >>> >> dict object, >> >>> > > assuming the headers are all uppercase is problematic and error >>> >> prone. For >> >>> > > example, I've seen X-Goog-Channel-Token in places, but IIRC header >>> >> case is >> >>> not >>> > > relevant. >>> > > >>> > > An easy solution (requiring no external libs) would be to change >>> >> all the >> >>> keys >>> > in >>> > > "headers" to uppercase before doing a lookup or create a secondary >>> >> mapping >> >>> the >>> > > all uppercase version to the header/headers in the original dict. >>> > >>> >> >> >> Done. >> >> https://codereview.appspot.**com/9885043/<https://codereview.appspot.com/9885... >> > > > > -- > Danny Hermes > Developer Programs Engineer >
Sign in to reply to this message.
Some kind of verification is one of the examples I've heard about. Another example was for clients to store a little bit of state in it (a session ID, a user id, etc)... I can see that being very useful for the case where the client creating the subscription is not the same as the client receiving it. For this case, I think it's reasonable for the Update method to clobber the token field. When everything is WAI, the API server should always echo the same thing back. If the client really wants to use it for verification, it can verify it before calling Update. On Tue, Jun 25, 2013 at 3:21 PM, Joe Gregorio <jcgregorio@google.com> wrote: > Yeah, at one point it was supposed to be used for verification, now it's > just a place to > store extra info that gets passed back to the caller. But, I will point > out, I don't disagree, > it doesn't make much sense. > > > On Tue, Jun 25, 2013 at 3:16 PM, Danny Hermes <dhermes@google.com> wrote: > >> So what is the point of the token? I guess I missed that discussion. >> >> >> On Tue, Jun 25, 2013 at 10:48 AM, <jcgregorio@google.com> wrote: >> >>> >>> https://codereview.appspot.**com/9885043/diff/14001/** >>> apiclient/channel.py<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py> >>> File apiclient/channel.py (right): >>> >>> https://codereview.appspot.**com/9885043/diff/14001/** >>> apiclient/channel.py#**newcode196<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode196> >>> apiclient/channel.py:196: setattr(self, param_name, value) >>> Per the previous discussion, the id uuid is supposed to be used for >>> security, not the token. >>> >>> >>> On 2013/06/13 16:14:55, dhermes wrote: >>> >>>> So you could send your watch request with a "token" which you use for >>>> >>> security >>> >>>> reasons and then watch sends you back a different value, so the one >>>> >>> you're using >>> >>>> for security is no longer used? >>>> >>> >>> On 2013/06/13 15:04:18, jcgregorio_google wrote: >>>> > No, they could all be overwritten. >>>> > >>>> > On 2013/06/03 20:26:59, dhermes wrote: >>>> > > Are there no attrs which should not be overwritten and/or cause an >>>> >>> error if >>> >>>> > the >>>> > > value changes? e.g. it seems like "token" is in place for security >>>> >>> and so >>> >>>> > > changing that value would be no bueno. >>>> > >>>> >>> >>> >>> https://codereview.appspot.**com/9885043/diff/14001/** >>> apiclient/channel.py#**newcode215<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode215> >>> apiclient/channel.py:215: event_token = headers['X-GOOG-CHANNEL-TOKEN'** >>> ] >>> On 2013/06/13 16:14:55, dhermes wrote: >>> >>>> You still haven't made these globals. Could you do that? >>>> >>> >>> On 2013/06/13 15:04:18, jcgregorio_google wrote: >>>> > Headers on the wire can be any case, but in most places they are >>>> >>> uppercased, >>> >>>> for >>>> > example, by the CGI spec and webapp2. Sadly this isn't universal so >>>> >>> yeah, I'm >>> >>>> > going to have to add defensive code here. You'd think in the year >>>> >>> 2013 this >>> >>>> > would be a solved problem in Python. >>>> > >>>> > On 2013/06/03 20:26:59, dhermes wrote: >>>> > > These headers should be globals. Also, if "headers" is a naive >>>> >>> dict object, >>> >>>> > > assuming the headers are all uppercase is problematic and error >>>> >>> prone. For >>> >>>> > > example, I've seen X-Goog-Channel-Token in places, but IIRC header >>>> >>> case is >>> >>>> not >>>> > > relevant. >>>> > > >>>> > > An easy solution (requiring no external libs) would be to change >>>> >>> all the >>> >>>> keys >>>> > in >>>> > > "headers" to uppercase before doing a lookup or create a secondary >>>> >>> mapping >>> >>>> the >>>> > > all uppercase version to the header/headers in the original dict. >>>> > >>>> >>> >>> >>> Done. >>> >>> https://codereview.appspot.**com/9885043/<https://codereview.appspot.com/9885... >>> >> >> >> >> -- >> Danny Hermes >> Developer Programs Engineer >> > > -- David Aristizabal | Test Engineer | darist@google.com | +1 917-410-1328
Sign in to reply to this message.
Alrighty then. LGTM. On Tue, Jun 25, 2013 at 12:31 PM, David Aristizabal <darist@google.com>wrote: > Some kind of verification is one of the examples I've heard about. > Another example was for clients to store a little bit of state in it (a > session ID, a user id, etc)... I can see that being very useful for the > case where the client creating the subscription is not the same as the > client receiving it. > > For this case, I think it's reasonable for the Update method to clobber > the token field. When everything is WAI, the API server should always echo > the same thing back. If the client really wants to use it for verification, > it can verify it before calling Update. > > > On Tue, Jun 25, 2013 at 3:21 PM, Joe Gregorio <jcgregorio@google.com>wrote: > >> Yeah, at one point it was supposed to be used for verification, now it's >> just a place to >> store extra info that gets passed back to the caller. But, I will point >> out, I don't disagree, >> it doesn't make much sense. >> >> >> On Tue, Jun 25, 2013 at 3:16 PM, Danny Hermes <dhermes@google.com> wrote: >> >>> So what is the point of the token? I guess I missed that discussion. >>> >>> >>> On Tue, Jun 25, 2013 at 10:48 AM, <jcgregorio@google.com> wrote: >>> >>>> >>>> https://codereview.appspot.**com/9885043/diff/14001/** >>>> apiclient/channel.py<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py> >>>> File apiclient/channel.py (right): >>>> >>>> https://codereview.appspot.**com/9885043/diff/14001/** >>>> apiclient/channel.py#**newcode196<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode196> >>>> apiclient/channel.py:196: setattr(self, param_name, value) >>>> Per the previous discussion, the id uuid is supposed to be used for >>>> security, not the token. >>>> >>>> >>>> On 2013/06/13 16:14:55, dhermes wrote: >>>> >>>>> So you could send your watch request with a "token" which you use for >>>>> >>>> security >>>> >>>>> reasons and then watch sends you back a different value, so the one >>>>> >>>> you're using >>>> >>>>> for security is no longer used? >>>>> >>>> >>>> On 2013/06/13 15:04:18, jcgregorio_google wrote: >>>>> > No, they could all be overwritten. >>>>> > >>>>> > On 2013/06/03 20:26:59, dhermes wrote: >>>>> > > Are there no attrs which should not be overwritten and/or cause an >>>>> >>>> error if >>>> >>>>> > the >>>>> > > value changes? e.g. it seems like "token" is in place for security >>>>> >>>> and so >>>> >>>>> > > changing that value would be no bueno. >>>>> > >>>>> >>>> >>>> >>>> https://codereview.appspot.**com/9885043/diff/14001/** >>>> apiclient/channel.py#**newcode215<https://codereview.appspot.com/9885043/diff/14001/apiclient/channel.py#newcode215> >>>> apiclient/channel.py:215: event_token = headers['X-GOOG-CHANNEL-TOKEN'* >>>> *] >>>> On 2013/06/13 16:14:55, dhermes wrote: >>>> >>>>> You still haven't made these globals. Could you do that? >>>>> >>>> >>>> On 2013/06/13 15:04:18, jcgregorio_google wrote: >>>>> > Headers on the wire can be any case, but in most places they are >>>>> >>>> uppercased, >>>> >>>>> for >>>>> > example, by the CGI spec and webapp2. Sadly this isn't universal so >>>>> >>>> yeah, I'm >>>> >>>>> > going to have to add defensive code here. You'd think in the year >>>>> >>>> 2013 this >>>> >>>>> > would be a solved problem in Python. >>>>> > >>>>> > On 2013/06/03 20:26:59, dhermes wrote: >>>>> > > These headers should be globals. Also, if "headers" is a naive >>>>> >>>> dict object, >>>> >>>>> > > assuming the headers are all uppercase is problematic and error >>>>> >>>> prone. For >>>> >>>>> > > example, I've seen X-Goog-Channel-Token in places, but IIRC header >>>>> >>>> case is >>>> >>>>> not >>>>> > > relevant. >>>>> > > >>>>> > > An easy solution (requiring no external libs) would be to change >>>>> >>>> all the >>>> >>>>> keys >>>>> > in >>>>> > > "headers" to uppercase before doing a lookup or create a secondary >>>>> >>>> mapping >>>> >>>>> the >>>>> > > all uppercase version to the header/headers in the original dict. >>>>> > >>>>> >>>> >>>> >>>> Done. >>>> >>>> https://codereview.appspot.**com/9885043/<https://codereview.appspot.com/9885... >>>> >>> >>> >>> >>> -- >>> Danny Hermes >>> Developer Programs Engineer >>> >> >> > > > -- > > David Aristizabal | Test Engineer | darist@google.com | +1 917-410-1328 > -- Danny Hermes Developer Programs Engineer
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
