Add messaging send_all and send_multicast functions#283
Add messaging send_all and send_multicast functions#283hiranya911 merged 8 commits intofirebase:masterfrom
Conversation
e5013c8 to
7bf84b4
Compare
|
Thanks @ZachOrr for working on this. We actually have an approved API design for this. It goes something like this: You might want to look into using the Google API client for making the batch requests: https://developers.google.com/api-client-library/python/guide/batch |
85fe93f to
844207e
Compare
8511957 to
15e7e87
Compare
ZachOrr
left a comment
There was a problem hiding this comment.
@hiranya911 Thanks for the guidance! I think I'm getting closer on this one. I left some comments with specific questions. Could you take a look when you get a chance?
|
Hi @ZachOrr. From what I can tell |
15e7e87 to
544877a
Compare
|
Awesome! Thanks for the help. It seems like using |
|
@ZachOrr yes, lets do that for now. I'll see if we can find a better alternative for that part. |
544877a to
13de0ff
Compare
|
Added tests and added errors raises from batch request http errors. |
4bd6a15 to
8cc100e
Compare
8cc100e to
fd6822c
Compare
|
Not entirely sure what the fix is to get that |
hiranya911
left a comment
There was a problem hiding this comment.
I think this is shaping up pretty nicely. As for the test failure, it seems the response payload is coming through as a bytes object. So we'll need to decode it first.
json.loads(response.decode())
firebase_admin/messaging.py
Outdated
| if resp.status == 200: | ||
| return json.loads(body) | ||
| else: | ||
| raise Exception('unexpected response') |
There was a problem hiding this comment.
I used this in my hack. But we need to raise something more detailed here. I think we need to parse the error response body, and construct a messaging.ApiCallError.
There was a problem hiding this comment.
Is there a situation where FCM will return a non-200 or non-error response code? I'm trying to figure out what to do here. A messaging.ApiCallError takes an error, which we really don't have in this case.
There was a problem hiding this comment.
Technically it can be any HTTP response. But in practice FCM mostly returns 200 or 4xx/5xx error codes. We can treat anything that is not 200 as an error.
We need to somehow call the error parsing logic in _handle_fcm_error() on this, and get an ApiCallError out of it. May be refactor the parsing logic into a new helper function:
if resp.status != 200:
code, message = _parse_error_response(response)
raise ApiCallError(code, message) # last argument is optional
e051a7c to
10b4e91
Compare
10b4e91 to
97d0b84
Compare
|
Resolves #277 |
a44e39e to
07bd5ac
Compare
93a7dd4 to
834bf5a
Compare
834bf5a to
ecec7fa
Compare
|
Alright - I think this is ready for a re-review. |
hiranya911
left a comment
There was a problem hiding this comment.
Thanks @ZachOrr. I think something is broken somewhere. There are a bunch of tests where the API raises exceptions, where it should not. Looks like the code is not handling some case correctly.
| _ = self._instrument_batch_messaging_service( | ||
| payload=self._batch_payload([(200, success_payload), (status, error_payload)])) | ||
| msg = messaging.Message(topic='foo') | ||
| batch_response = messaging.send_all([msg, msg]) |
There was a problem hiding this comment.
Now I'm confused. Why wouldn't this throw? It is the right behavior, but how is this any different from the test cases above?
|
I think I have everything addressed so far. |
|
Thanks @ZachOrr. Looks good. Please fix the lint error detected by CI, and then I can approve/merge this. You can add |
|
🙌 Got the lint passing. Thanks @hiranya911 |
|
Thanks @ZachOrr. I'm going to merge this now. I will try and get it released next week. We also need to add 1-2 integration test cases to further verify this. I can work on it, or you're also welcome to provide a PR for that. |
Hey y'all - I'm working on adding the
sendAllandsendMulticastfeatures for #196 based on firebase/firebase-admin-node#453 - I was hoping I could get some assistance with how to represent returns from these functions, since the currentsendendpoint will raise if the response from FCM contains an error, and this doesn't seem like a great solution when sending several requests.Also, if I could get suggestions on a non-regex way to wrangle batch response content, that would be appreciated. My first thought was seeing if we could dump each individual response content in to a requests Response object, but that doesn't appear to be a thing with Response objects.