Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add messaging send_all and send_multicast functions #283

Merged
merged 8 commits into from May 13, 2019

Conversation

@ZachOrr
Copy link
Contributor

@ZachOrr ZachOrr commented May 1, 2019

Hey y'all - I'm working on adding the sendAll and sendMulticast features 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 current send endpoint 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.

@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch from e5013c8 to 7bf84b4 May 1, 2019
@hiranya911
Copy link
Member

@hiranya911 hiranya911 commented May 1, 2019

Thanks @ZachOrr for working on this. We actually have an approved API design for this. It goes something like this:

firebase_admin.messaging
  send_all(messages, dry_run=False, app=None): BatchResponse
  send_multicast(multicast_message, dry_run=False, app=None): BatchResponse

  class MulticastMessage
    tokens: list<str>
    Everything else on Message class except token, topic and condition

  class BatchResponse
    responses: list<SendResponse>
    success_count: int
    failure_count: int

  class SendResponse
    success: boolean
    message_id: str
    exception: ApiCallError

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

Loading

@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch from 85fe93f to 844207e May 2, 2019
@ZachOrr ZachOrr changed the title [WIP] Add messaging sendAll and sendMulticast function [WIP] Add messaging send_all and send_multicast functions May 2, 2019
@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch 4 times, most recently from 8511957 to 15e7e87 May 2, 2019
Copy link
Contributor Author

@ZachOrr ZachOrr left a 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?

Loading

firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/_messaging_utils.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/_messaging_utils.py Outdated Show resolved Hide resolved
Loading
firebase_admin/_messaging_utils.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
requirements.txt Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
@hiranya911
Copy link
Member

@hiranya911 hiranya911 commented May 2, 2019

Hi @ZachOrr. From what I can tell googleapiclient and googe.auth do not play well with each other in Python. This is unfortunate, as they really work well in other languages like Java and C#. In any case, I managed to send a batch request using the following code:

import json

import googleapiclient
from googleapiclient import http
from googleapiclient import _auth
import google.auth

# Init Google Credentials (this part is currently handled by firebase_admin.credentials)
_scopes = [
    'https://www.googleapis.com/auth/cloud-platform',
    'https://www.googleapis.com/auth/datastore',
    'https://www.googleapis.com/auth/devstorage.read_write',
    'https://www.googleapis.com/auth/firebase',
    'https://www.googleapis.com/auth/identitytoolkit',
    'https://www.googleapis.com/auth/userinfo.email'
]
creds, _ = google.auth.default(scopes=_scopes)

# It sucks that we have to use this internal _auth module. Wish there was a better way
transport = _auth.authorized_http(creds)

responses = []

def callback(req_id, resp, exception):
  print(req_id, resp, exception)
  responses.append(resp['name'])

def postproc(resp, body):
  """This gets called for each part. It's a low-level function where we can parse the HTTP response."""
  if resp.status == 200:
    return json.loads(body)
  else:
    raise Exception('unexpected response')

batch = http.BatchHttpRequest(callback, 'https://fcm.googleapis.com/batch')

body = json.dumps({'message': {'topic': 'foo'}})
req = http.HttpRequest(
  http=transport, postproc=postproc,
  uri='https://fcm.googleapis.com/v1/projects/my-project-id/messages:send',
  method='POST',
  body=body)

# Queue 3 requests in the batch
batch.add(req)
batch.add(req)
batch.add(req)

batch.execute()
print(responses) # Prints 3 message IDs

Loading

@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch from 15e7e87 to 544877a May 2, 2019
@ZachOrr
Copy link
Contributor Author

@ZachOrr ZachOrr commented May 2, 2019

Awesome! Thanks for the help. It seems like using app.credential.get_credential() in _auth.authorized_http works fine - can we do that?

Loading

@hiranya911
Copy link
Member

@hiranya911 hiranya911 commented May 2, 2019

@ZachOrr yes, lets do that for now. I'll see if we can find a better alternative for that part.

Loading

@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch from 544877a to 13de0ff May 3, 2019
@ZachOrr ZachOrr changed the title [WIP] Add messaging send_all and send_multicast functions Add messaging send_all and send_multicast functions May 3, 2019
@ZachOrr ZachOrr marked this pull request as ready for review May 3, 2019
@ZachOrr
Copy link
Contributor Author

@ZachOrr ZachOrr commented May 3, 2019

Added tests and added errors raises from batch request http errors.

Loading

@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch 2 times, most recently from 4bd6a15 to 8cc100e May 3, 2019
@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch from 8cc100e to fd6822c May 3, 2019
@ZachOrr
Copy link
Contributor Author

@ZachOrr ZachOrr commented May 3, 2019

Not entirely sure what the fix is to get that json.loads working on some versions of Python. Any suggestions?

Loading

Copy link
Member

@hiranya911 hiranya911 left a 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())

Loading

firebase_admin/_messaging_utils.py Outdated Show resolved Hide resolved
Loading
firebase_admin/_messaging_utils.py Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
if resp.status == 200:
return json.loads(body)
else:
raise Exception('unexpected response')
Copy link
Member

@hiranya911 hiranya911 May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Contributor Author

@ZachOrr ZachOrr May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@hiranya911 hiranya911 May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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  

Loading

tests/test_messaging.py Show resolved Hide resolved
Loading
tests/test_messaging.py Show resolved Hide resolved
Loading
tests/test_messaging.py Show resolved Hide resolved
Loading
@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch 4 times, most recently from e051a7c to 10b4e91 May 3, 2019
@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch from 10b4e91 to 97d0b84 May 3, 2019
@hiranya911
Copy link
Member

@hiranya911 hiranya911 commented May 3, 2019

Resolves #277

Loading

@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch from a44e39e to 07bd5ac May 4, 2019
@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch from 93a7dd4 to 834bf5a May 4, 2019
@ZachOrr ZachOrr force-pushed the messages-add-batch-send branch from 834bf5a to ecec7fa May 4, 2019
@ZachOrr
Copy link
Contributor Author

@ZachOrr ZachOrr commented May 4, 2019

Alright - I think this is ready for a re-review.

Loading

Copy link
Member

@hiranya911 hiranya911 left a 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.

Loading

tests/test_messaging.py Outdated Show resolved Hide resolved
Loading
tests/test_messaging.py Outdated Show resolved Hide resolved
Loading
tests/test_messaging.py Outdated Show resolved Hide resolved
Loading
tests/test_messaging.py Outdated Show resolved Hide resolved
Loading
_ = 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])
Copy link
Member

@hiranya911 hiranya911 May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

tests/test_messaging.py Outdated Show resolved Hide resolved
Loading
tests/test_messaging.py Outdated Show resolved Hide resolved
Loading
tests/test_messaging.py Outdated Show resolved Hide resolved
Loading
tests/test_messaging.py Outdated Show resolved Hide resolved
Loading
firebase_admin/messaging.py Show resolved Hide resolved
Loading
firebase_admin/messaging.py Outdated Show resolved Hide resolved
Loading
@hiranya911 hiranya911 self-assigned this May 8, 2019
@ZachOrr
Copy link
Contributor Author

@ZachOrr ZachOrr commented May 12, 2019

I think I have everything addressed so far.

Loading

@hiranya911
Copy link
Member

@hiranya911 hiranya911 commented May 13, 2019

Thanks @ZachOrr. Looks good. Please fix the lint error detected by CI, and then I can approve/merge this.

You can add delete resp to the method to deal with unused args.

Loading

@ZachOrr
Copy link
Contributor Author

@ZachOrr ZachOrr commented May 13, 2019

🙌 Got the lint passing. Thanks @hiranya911

Loading

Copy link
Member

@hiranya911 hiranya911 left a comment

LGTM 👍

Loading

@hiranya911
Copy link
Member

@hiranya911 hiranya911 commented May 13, 2019

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.

Loading

@hiranya911 hiranya911 merged commit 8f71485 into firebase:master May 13, 2019
2 checks passed
Loading
@ZachOrr ZachOrr deleted the messages-add-batch-send branch May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants