-
Notifications
You must be signed in to change notification settings - Fork 30
feat: Add ChannelID report for UAID #847
Conversation
Codecov Report
@@ Coverage Diff @@
## master #847 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 48 48
Lines 9470 9511 +41
=====================================
+ Hits 9470 9511 +41
Continue to review full report at Codecov.
|
autopush/db.py
Outdated
@@ -495,7 +495,8 @@ def all_channels(self, uaid): | |||
# Functions that call store_message() would be required to | |||
# update that list as well using register_channel() | |||
try: | |||
result = self.table.get_item(consistent=True, uaid=hasher(uaid), | |||
result = self.table.get_item(consistent=True, | |||
uaid=hasher(str(uaid)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
str again? The type sig indicates its already a str? Does that mean its being used incorrectly somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to restrict this call to router_type in ('gcm', 'apns')?
autopush/web/registration.py
Outdated
def _list_channels(self): | ||
_, channels = self.ap_settings.message.all_channels(self.uaid) | ||
dashed = [str(uuid.UUID(x)) for x in channels] | ||
self.write(json.dumps( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_list_channels is called in a separate thread so you really want to go back to the event loop for write/finish
autopush/web/registration.py
Outdated
_, channels = self.ap_settings.message.all_channels(self.uaid) | ||
dashed = [str(uuid.UUID(x)) for x in channels] | ||
self.write(json.dumps( | ||
{"uaid": self.uaid.hex, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: returning uaid's superfluous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily. For some "state challenged" remote calls, it's not always easy to associate a response back to it's request. Having a shared ID between the two can simplify things. In this case, the UAID is required for the URL and the response, plus it's how the client organizes.
@pjenvey Well, they need to have a "secret" bearer token which is only provided when calling HTTP registration (not websocket), so it's already limited. |
autopush/web/registration.py
Outdated
@@ -307,6 +337,15 @@ def _return_endpoint(self, endpoint_data, new_uaid, router=None): | |||
client_info=self._client_info) | |||
self.finish() | |||
|
|||
def _list_channels(self, uaid): | |||
_, channels = self.ap_settings.message.all_channels(str(uaid)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this db call should definitely be in a thread though, we just don't really want the write/finish in one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, misread how Deferred() works. switch to deferToThread()
0413e78
to
47c0787
Compare
autopush/web/registration.py
Outdated
|
||
""" | ||
self.uaid = self.valid_input['uaid'] | ||
if not self.uaid: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation doesn't actually validate what is required (the UAID in this case)? We added validation schema's to avoid if/else checks in the code here.
Adds new bridge system HTTP endpoint to fetch a list of server known CHIDs for a given UAID. Useful for remote clients to check status. (see documentation for calling structure and return). Also fixed an edge case where clients that may have been forced offline due to external system errors could still register new endpoints. The behavior will now be to return a 410 error if a client has been flagged as disconnected by the server. It is up to the client to check the local bridge connection and re-establish any endpoints. closes: #844, #843
Adds new bridge system HTTP endpoint to fetch a list of server known
CHIDs for a given UAID. Useful for remote clients to check status. (see
documentation for calling structure and return).
Also fixed an edge case where clients that may have been forced offline
due to external system errors could still register new endpoints. The
behavior will now be to return a 410 error if a client has been flagged
as disconnected by the server. It is up to the client to check the local
bridge connection and re-establish any endpoints.
closes: #844, #843