Skip to content

WIP feat(fcm): Add support for AndroidConfigV2#968

Draft
jonathanedey wants to merge 3 commits into
mainfrom
je-fcm-v2
Draft

WIP feat(fcm): Add support for AndroidConfigV2#968
jonathanedey wants to merge 3 commits into
mainfrom
je-fcm-v2

Conversation

@jonathanedey

Copy link
Copy Markdown
Contributor

This change implements support for the Android v2 API and it deprecates v1 AndroidConfig structures and replaces them with the new AndroidConfigV2 structures.

Adds AndroidConfigV2, AndroidNotificationV2, AndroidRemoteNotification, and AndroidBackgroundSyncMessage data classes.
Deprecates AndroidConfig and AndroidNotification.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces V2 configurations for Android messaging in the Firebase Admin Python SDK, including AndroidConfigV2, AndroidNotificationV2, AndroidRemoteNotification, and AndroidBackgroundSyncMessage, while deprecating the older V1 equivalents. The feedback highlights an issue where vibrate_timings_millis is documented to support datetime.timedelta instances, but the implementation only validates numbers. To resolve this, the reviewer suggests adding a helper method to validate both types, updating the encoder, and adjusting the test suite to verify this behavior.

Comment thread firebase_admin/_messaging_encoder.py
Comment thread firebase_admin/_messaging_encoder.py Outdated
Comment thread tests/test_messaging.py Outdated
Comment thread tests/test_messaging.py
@jonathanedey

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the new AndroidConfigV2 and its associated V2 classes (AndroidNotificationV2, AndroidRemoteNotification, and AndroidBackgroundSyncMessage) to the FCM messaging module, while deprecating the older AndroidConfig and AndroidNotification implementations. It includes comprehensive encoding, validation logic, and unit tests. The review feedback highlights a few validation improvements: specifically, explicitly rejecting boolean values in check_vibrate_timings (since Python treats bool as a subclass of int/numbers.Number) and ensuring that all boolean fields in AndroidNotificationV2 are properly validated using _Validators.check_boolean.

Comment on lines +237 to +240
non_valid = [
k for k in value
if not isinstance(k, (numbers.Number, datetime.timedelta))
]

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

In Python, bool is a subclass of int (and thus numbers.Number). Therefore, isinstance(True, numbers.Number) evaluates to True. This means boolean values like True or False will incorrectly pass the check_vibrate_timings validation and be treated as valid vibrate timings (which are then encoded as 1s or 0s by encode_milliseconds). We should explicitly reject boolean values.

Suggested change
non_valid = [
k for k in value
if not isinstance(k, (numbers.Number, datetime.timedelta))
]
non_valid = [
k for k in value
if isinstance(k, bool) or not isinstance(k, (numbers.Number, datetime.timedelta))
]

Comment on lines +586 to +589
'sticky': notification.sticky,
'event_time': _Validators.check_datetime(
'AndroidNotificationV2.event_time', notification.event_time),
'local_only': notification.local_only,

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

The fields sticky and local_only in AndroidNotificationV2 are boolean fields but are not validated using _Validators.check_boolean. To ensure type safety and prevent invalid types from being serialized and sent to the FCM backend, we should validate them.

            'sticky': _Validators.check_boolean(
                'AndroidNotificationV2.sticky', notification.sticky),
            'event_time': _Validators.check_datetime(
                'AndroidNotificationV2.event_time', notification.event_time),
            'local_only': _Validators.check_boolean(
                'AndroidNotificationV2.local_only', notification.local_only),

Comment on lines +596 to +598
'default_vibrate_timings': notification.default_vibrate_timings,
'default_sound': notification.default_sound,
'default_light_settings': notification.default_light_settings,

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

The fields default_vibrate_timings, default_sound, and default_light_settings in AndroidNotificationV2 are boolean fields but are not validated using _Validators.check_boolean. We should validate them to ensure type safety and prevent invalid types from being serialized.

Suggested change
'default_vibrate_timings': notification.default_vibrate_timings,
'default_sound': notification.default_sound,
'default_light_settings': notification.default_light_settings,
'default_vibrate_timings': _Validators.check_boolean(
'AndroidNotificationV2.default_vibrate_timings', notification.default_vibrate_timings),
'default_sound': _Validators.check_boolean(
'AndroidNotificationV2.default_sound', notification.default_sound),
'default_light_settings': _Validators.check_boolean(
'AndroidNotificationV2.default_light_settings', notification.default_light_settings),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant