Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions firebase_admin/_messaging_encoder.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import math
import numbers
import re
import warnings

from firebase_admin import _messaging_utils

Expand All @@ -27,7 +28,7 @@ class Message:
"""A message that can be sent via Firebase Cloud Messaging.

Contains payload information as well as recipient information. In particular, the message must
contain exactly one of token, topic or condition fields.
contain exactly one of fid, token, topic or condition fields.

Args:
data: A dictionary of data fields (optional). All keys and values in the dictionary must be
Expand All @@ -37,20 +38,29 @@ class Message:
webpush: An instance of ``messaging.WebpushConfig`` (optional).
apns: An instance of ``messaging.ApnsConfig`` (optional).
fcm_options: An instance of ``messaging.FCMOptions`` (optional).
token: The registration token of the device to which the message should be sent (optional).
fid: The Firebase installation ID of an FCM registered app instance to which the
message should be sent (optional)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: missing . at the end

token: Deprecated. Use ``fid`` instead.
topic: Name of the FCM topic to which the message should be sent (optional). Topic name
may contain the ``/topics/`` prefix.
condition: The FCM condition to which the message should be sent (optional).
"""

def __init__(self, data=None, notification=None, android=None, webpush=None, apns=None,
fcm_options=None, token=None, topic=None, condition=None):
fcm_options=None, fid=None, token=None, topic=None, condition=None):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's move fid=None to end of the list to remove any potential backward compatibility issues if someone is currently passing arguments positionally rather than by keyword.

def __init__(self, data=None, notification=None, android=None, webpush=None, apns=None,
             fcm_options=None, token=None, topic=None, condition=None, fid=None):

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

if token is not None:
warnings.warn(
"Deprecated. Use 'fid' instead.",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's change this to Message.token is deprecated. Use fid instead.

Suggested change
"Deprecated. Use 'fid' instead.",
"Message.token is deprecated. Use fid instead.",

DeprecationWarning,
stacklevel=2
)
self.data = data
self.notification = notification
self.android = android
self.webpush = webpush
self.apns = apns
self.fcm_options = fcm_options
self.fid = fid
self.token = token
self.topic = topic
self.condition = condition
Expand All @@ -60,10 +70,11 @@ def __str__(self):


class MulticastMessage:
"""A message that can be sent to multiple tokens via Firebase Cloud Messaging.
"""A message that can be sent to multiple tokens or fids via Firebase Cloud Messaging.

Args:
tokens: A list of registration tokens of targeted devices.
fids: A list of Firebase Installation IDs of targeted app instances (optional)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: missing . at the end

tokens: Deprecated. Use ``fids`` instead (optional).
data: A dictionary of data fields (optional). All keys and values in the dictionary must be
strings.
notification: An instance of ``messaging.Notification`` (optional).
Expand All @@ -72,12 +83,31 @@ class MulticastMessage:
apns: An instance of ``messaging.ApnsConfig`` (optional).
fcm_options: An instance of ``messaging.FCMOptions`` (optional).
"""
def __init__(self, tokens, data=None, notification=None, android=None, webpush=None, apns=None,
def __init__(self, fids=None, tokens=None, data=None, notification=None, android=None, webpush=None, apns=None,
fcm_options=None):
_Validators.check_string_list('MulticastMessage.tokens', tokens)
if len(tokens) > 500:
raise ValueError('MulticastMessage.tokens must not contain more than 500 tokens.')
self.tokens = tokens
if tokens is not None:
warnings.warn(
"Deprecated. Use 'fids' instead.",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"Deprecated. Use 'fids' instead.",
"MulticastMessage.tokens is deprecated. Use fids instead.",

DeprecationWarning,
stacklevel=2
)

if (tokens is None and fids is None) or (tokens is not None and fids is not None):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to support the case where the tokens is not None and fids is not None? We can raise an error if len(tokens) + len(fids) > 500 in this case.

Supporting this case will allow people to start using fids even when not all of their tokens have been switched to fids.

With that being said, this is just a nice-to-have. It's unnecessary if it requires too many changes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed the codes to support the mix of tokens and fids for MulticastMessage. But I removed the error messages for checking the number of tokens or fids exceeding 500 individually since I feel that's a bit redundant. Please let me know if you want us to revert that.

raise ValueError("Must specify either 'tokens' or 'fids'.")

if tokens is not None:
_Validators.check_string_list('MulticastMessage.tokens', tokens)
if len(tokens) > 500:
raise ValueError('MulticastMessage.tokens must not contain more than 500 tokens.')
self.tokens = tokens
self.fids = None
else:
_Validators.check_string_list('MulticastMessage.fids', fids)
if len(fids) > 500:
raise ValueError('MulticastMessage.fids must not contain more than 500 fids.')
self.fids = fids
self.tokens = None

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The change to the MulticastMessage.__init__ method signature introduces a significant backward-incompatible change. The new signature is def __init__(self, fids=None, tokens=None, ...) where previously it was def __init__(self, tokens, ...). This breaks calls that use positional arguments, as MulticastMessage(['my-token']) will now silently and incorrectly interpret ['my-token'] as a list of fids.

To avoid this breaking change, I suggest reverting the argument order and adjusting the logic to handle the mutual exclusivity of tokens and fids. The tokens parameter should be made optional, and fids can be added at the end. This ensures backward compatibility for existing users.

    def __init__(self, tokens=None, data=None, notification=None, android=None, webpush=None, apns=None,
                 fcm_options=None, fids=None):
        if tokens is not None:
            warnings.warn(
                "Deprecated. Use 'fids' instead.",
                DeprecationWarning,
                stacklevel=2
            )

        if (tokens is not None) + (fids is not None) != 1:
            raise ValueError("Must specify exactly one of 'tokens' or 'fids'.")

        if tokens is not None:
            _Validators.check_string_list('MulticastMessage.tokens', tokens)
            if len(tokens) > 500:
                raise ValueError('MulticastMessage.tokens must not contain more than 500 tokens.')
            self.tokens = tokens
            self.fids = None
        else:
            _Validators.check_string_list('MulticastMessage.fids', fids)
            if len(fids) > 500:
                raise ValueError('MulticastMessage.fids must not contain more than 500 fids.')
            self.fids = fids
            self.tokens = None

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed


self.data = data
self.notification = notification
self.android = android
Expand Down Expand Up @@ -695,16 +725,17 @@ def default(self, o): # pylint: disable=method-hidden
'Message.condition', o.condition, non_empty=True),
'data': _Validators.check_string_dict('Message.data', o.data),
'notification': MessageEncoder.encode_notification(o.notification),
'fid': _Validators.check_string('Message.fid', o.fid, non_empty=True),
'token': _Validators.check_string('Message.token', o.token, non_empty=True),
'topic': _Validators.check_string('Message.topic', o.topic, non_empty=True),
'webpush': MessageEncoder.encode_webpush(o.webpush),
'fcm_options': MessageEncoder.encode_fcm_options(o.fcm_options),
}
result['topic'] = MessageEncoder.sanitize_topic_name(result.get('topic'))
result = MessageEncoder.remove_null_values(result)
target_count = sum(t in result for t in ['token', 'topic', 'condition'])
target_count = sum(t in result for t in ['fid', 'token', 'topic', 'condition'])
if target_count != 1:
raise ValueError('Exactly one of token, topic or condition must be specified.')
raise ValueError('Exactly one of fid, token, topic or condition must be specified.')
return result

@classmethod
Expand Down
67 changes: 47 additions & 20 deletions firebase_admin/messaging.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import json
import asyncio
import logging
import warnings
import requests
import httpx

Expand Down Expand Up @@ -177,7 +178,7 @@ async def send_each_for_multicast_async(
dry_run: bool = False,
app: Optional[App] = None
) -> BatchResponse:
"""Sends the given mutlicast message to each token asynchronously via Firebase Cloud Messaging
"""Sends the given multicast message to each token or fid asynchronously via Firebase Cloud Messaging
(FCM).

If the ``dry_run`` mode is enabled, the message will not be actually delivered to the
Expand All @@ -197,19 +198,32 @@ async def send_each_for_multicast_async(
"""
if not isinstance(multicast_message, MulticastMessage):
raise ValueError('Message must be an instance of messaging.MulticastMessage class.')
messages = [Message(
data=multicast_message.data,
notification=multicast_message.notification,
android=multicast_message.android,
webpush=multicast_message.webpush,
apns=multicast_message.apns,
fcm_options=multicast_message.fcm_options,
token=token
) for token in multicast_message.tokens]
if multicast_message.tokens is not None:
with warnings.catch_warnings():
warnings.simplefilter("ignore", DeprecationWarning)
messages = [Message(
data=multicast_message.data,
notification=multicast_message.notification,
android=multicast_message.android,
webpush=multicast_message.webpush,
apns=multicast_message.apns,
fcm_options=multicast_message.fcm_options,
token=token
) for token in multicast_message.tokens]
else:
messages = [Message(
data=multicast_message.data,
notification=multicast_message.notification,
android=multicast_message.android,
webpush=multicast_message.webpush,
apns=multicast_message.apns,
fcm_options=multicast_message.fcm_options,
fid=fid
) for fid in multicast_message.fids]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This block of logic for converting a MulticastMessage into a list of Message objects is duplicated in send_each_for_multicast (lines 245-266). To improve maintainability and follow the DRY (Don't Repeat Yourself) principle, consider extracting this logic into a private helper function that can be called from both send_each_for_multicast_async and send_each_for_multicast.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Helper function is added

return await _get_messaging_service(app).send_each_async(messages, dry_run)

def send_each_for_multicast(multicast_message, dry_run=False, app=None):
"""Sends the given mutlicast message to each token via Firebase Cloud Messaging (FCM).
"""Sends the given multicast message to each token or fid via Firebase Cloud Messaging (FCM).

If the ``dry_run`` mode is enabled, the message will not be actually delivered to the
recipients. Instead, FCM performs all the usual validations and emulates the send operation.
Expand All @@ -228,15 +242,28 @@ def send_each_for_multicast(multicast_message, dry_run=False, app=None):
"""
if not isinstance(multicast_message, MulticastMessage):
raise ValueError('Message must be an instance of messaging.MulticastMessage class.')
messages = [Message(
data=multicast_message.data,
notification=multicast_message.notification,
android=multicast_message.android,
webpush=multicast_message.webpush,
apns=multicast_message.apns,
fcm_options=multicast_message.fcm_options,
token=token
) for token in multicast_message.tokens]
if multicast_message.tokens is not None:
with warnings.catch_warnings():
warnings.simplefilter("ignore", DeprecationWarning)
messages = [Message(
data=multicast_message.data,
notification=multicast_message.notification,
android=multicast_message.android,
webpush=multicast_message.webpush,
apns=multicast_message.apns,
fcm_options=multicast_message.fcm_options,
token=token
) for token in multicast_message.tokens]
else:
messages = [Message(
data=multicast_message.data,
notification=multicast_message.notification,
android=multicast_message.android,
webpush=multicast_message.webpush,
apns=multicast_message.apns,
fcm_options=multicast_message.fcm_options,
fid=fid
) for fid in multicast_message.fids]
return _get_messaging_service(app).send_each(messages, dry_run)

def subscribe_to_topic(tokens, topic, app=None):
Expand Down
78 changes: 74 additions & 4 deletions tests/test_messaging.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,18 +73,22 @@ class TestMessageStr:
messaging.Message(topic='topic', condition='condition'),
messaging.Message(condition='condition', token='token'),
messaging.Message(topic='topic', token='token', condition='condition'),
messaging.Message(fid='fid', token='token'),
messaging.Message(fid='fid', topic='topic'),
messaging.Message(fid='fid', condition='condition'),
messaging.Message(fid='fid', token='token', topic='topic'),
])
def test_invalid_target_message(self, msg):
with pytest.raises(ValueError) as excinfo:
str(msg)
assert str(
excinfo.value) == 'Exactly one of token, topic or condition must be specified.'
excinfo.value) == 'Exactly one of fid, token, topic or condition must be specified.'

def test_empty_message(self):
assert str(messaging.Message(token='value')) == '{"token": "value"}'
assert str(messaging.Message(topic='value')) == '{"topic": "value"}'
assert str(messaging.Message(condition='value')
) == '{"condition": "value"}'
assert str(messaging.Message(condition='value')) == '{"condition": "value"}'
assert str(messaging.Message(fid='value')) == '{"fid": "value"}'

def test_data_message(self):
assert str(messaging.Message(topic='topic', data={})
Expand All @@ -95,6 +99,15 @@ def test_data_message(self):

class TestMulticastMessage:

def test_invalid_targets(self):
with pytest.raises(ValueError) as excinfo:
messaging.MulticastMessage()
assert str(excinfo.value) == "Must specify either 'tokens' or 'fids'."

with pytest.raises(ValueError) as excinfo:
messaging.MulticastMessage(tokens=['token'], fids=['fid'])
assert str(excinfo.value) == "Must specify either 'tokens' or 'fids'."

@pytest.mark.parametrize('tokens', NON_LIST_ARGS)
def test_invalid_tokens_type(self, tokens):
with pytest.raises(ValueError) as excinfo:
Expand All @@ -119,6 +132,34 @@ def test_tokens_type(self):
message = messaging.MulticastMessage(tokens=['token' for _ in range(0, 500)])
assert len(message.tokens) == 500

@pytest.mark.parametrize('fids', NON_LIST_ARGS)
def test_invalid_fids_type(self, fids):
with pytest.raises(ValueError) as excinfo:
messaging.MulticastMessage(fids=fids)
if isinstance(fids, list):
expected = 'MulticastMessage.fids must not contain non-string values.'
assert str(excinfo.value) == expected
else:
expected = 'MulticastMessage.fids must be a list of strings.'
assert str(excinfo.value) == expected

def test_fids_over_500(self):
with pytest.raises(ValueError) as excinfo:
messaging.MulticastMessage(fids=['fid' for _ in range(0, 501)])
expected = 'MulticastMessage.fids must not contain more than 500 fids.'
assert str(excinfo.value) == expected

def test_fids_type(self):
message = messaging.MulticastMessage(fids=['fid'])
assert len(message.fids) == 1

message = messaging.MulticastMessage(fids=['fid' for _ in range(0, 500)])
assert len(message.fids) == 500

def test_tokens_deprecation_warning(self):
with pytest.deprecated_call():
messaging.MulticastMessage(tokens=['token'])


class TestMessageEncoder:

Expand All @@ -128,18 +169,28 @@ class TestMessageEncoder:
messaging.Message(topic='topic', condition='condition'),
messaging.Message(condition='condition', token='token'),
messaging.Message(topic='topic', token='token', condition='condition'),
messaging.Message(fid='fid', token='token'),
messaging.Message(fid='fid', topic='topic'),
messaging.Message(fid='fid', condition='condition'),
messaging.Message(fid='fid', token='token', topic='topic'),
])
def test_invalid_target_message(self, msg):
with pytest.raises(ValueError) as excinfo:
check_encoding(msg)
assert str(excinfo.value) == 'Exactly one of token, topic or condition must be specified.'
assert str(excinfo.value) == 'Exactly one of fid, token, topic or condition must be specified.'

@pytest.mark.parametrize('target', NON_STRING_ARGS + [''])
def test_invalid_token(self, target):
with pytest.raises(ValueError) as excinfo:
check_encoding(messaging.Message(token=target))
assert str(excinfo.value) == 'Message.token must be a non-empty string.'

@pytest.mark.parametrize('target', NON_STRING_ARGS + [''])
def test_invalid_fid(self, target):
with pytest.raises(ValueError) as excinfo:
check_encoding(messaging.Message(fid=target))
assert str(excinfo.value) == 'Message.fid must be a non-empty string.'

@pytest.mark.parametrize('target', NON_STRING_ARGS + [''])
def test_invalid_topic(self, target):
with pytest.raises(ValueError) as excinfo:
Expand All @@ -159,9 +210,14 @@ def test_malformed_topic_name(self, topic):

def test_empty_message(self):
check_encoding(messaging.Message(token='value'), {'token': 'value'})
check_encoding(messaging.Message(fid='value'), {'fid': 'value'})
check_encoding(messaging.Message(topic='value'), {'topic': 'value'})
check_encoding(messaging.Message(condition='value'), {'condition': 'value'})

def test_token_deprecation_warning(self):
with pytest.deprecated_call():
messaging.Message(token='value')

@pytest.mark.parametrize('data', NON_DICT_ARGS)
def test_invalid_data_message(self, data):
with pytest.raises(ValueError):
Expand Down Expand Up @@ -2212,6 +2268,20 @@ def test_send_each_for_multicast(self):
assert all(r.success for r in batch_response.responses)
assert not any(r.exception for r in batch_response.responses)

def test_send_each_for_multicast_fids(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we have any tests for send_each_for_multicast_async?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We didn't have unit tests for send_each_for_multicast_async previously but only included it in integration tests. Added some unit tests with respx to mock raw HTTP/2 concurrent POST requests to Google's servers

payload1 = json.dumps({'name': 'message-id1'})
payload2 = json.dumps({'name': 'message-id2'})
_ = self._instrument_messaging_service(
response_dict={'foo1': [200, payload1], 'foo2': [200, payload2]})
msg = messaging.MulticastMessage(fids=['foo1', 'foo2'])
batch_response = messaging.send_each_for_multicast(msg, dry_run=True)
assert batch_response.success_count == 2
assert batch_response.failure_count == 0
assert len(batch_response.responses) == 2
assert [r.message_id for r in batch_response.responses] == ['message-id1', 'message-id2']
assert all(r.success for r in batch_response.responses)
assert not any(r.exception for r in batch_response.responses)

@pytest.mark.parametrize('status', HTTP_ERROR_CODES)
def test_send_each_for_multicast_detailed_error(self, status):
success_payload = json.dumps({'name': 'message-id'})
Expand Down
Loading