WIP feat(fcm): Add support for AndroidConfigV2 - #537
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces the Android V2 configuration classes (AndroidConfigV2, AndroidNotificationV2, AndroidRemoteNotification, and AndroidBackgroundSyncMessage) for Firebase Messaging, deprecating the older AndroidConfig and AndroidNotification classes. It updates Message and MulticastMessage to support the new AndroidV2 property, ensuring it is mutually exclusive with the legacy Android property, and adds corresponding unit tests and snippets. A critical issue was identified in AndroidConfigV2.cs where parsing a TTL string with fractional seconds that are all zeros (e.g., "1.0s") would result in an empty string being passed to long.Parse, causing a FormatException. A code suggestion was provided to handle this case safely.
Sorry, something went wrong.
|
/gemini review |
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces the Android V2 configuration (including AndroidConfigV2, AndroidNotificationV2, AndroidRemoteNotification, and AndroidBackgroundSyncMessage) to the Firebase Messaging module, deprecating the legacy AndroidConfig and AndroidNotification classes. It updates Message and MulticastMessage to support the new V2 properties, enforces mutual exclusivity between V1 and V2 configurations, and adds comprehensive unit tests and code snippets. Feedback on the changes highlights two key issues: first, a correctness bug in AndroidConfigV2.TtlString where fractional seconds are parsed incorrectly due to floating-point math and string manipulation, which can be resolved using TimeSpan.Ticks and integer-based parsing; second, a potential timezone conversion issue in AndroidNotificationV2.EventTimeString where parsing UTC timestamps with DateTimeStyles.None converts them to the local timezone, which should be corrected by using DateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversal.
Sorry, something went wrong.
…sing in AndroidConfigV2
Reviewers
1 more reviewerAt least 1 approving review is required to merge this pull request.
This change introduces FCM Android V2 messaging configuration support by adding AndroidConfigV2, AndroidNotificationV2, AndroidRemoteNotification, and AndroidBackgroundSyncMessage models to FirebaseAdmin.Messaging. The legacy Android property on Message and MulticastMessage has been marked as obsolete in favor of AndroidV2. Validation enforces mutual exclusivity between Android and AndroidV2 on Message, as well as requiring exactly one of RemoteNotification or BackgroundSync to be specified on AndroidConfigV2.
Additionally this change fixes a bug where fractional message durations (such as 1.5 seconds or 1.000000000 seconds) in Android V2 messages failed to parse or threw a formatting error. It also ensures notification event times in Android V2 are consistently handled in UTC time to match other Firebase SDKs, while keeping legacy V1 behavior unchanged for backward compatibility.
Added unit tests in MessageTest.cs and MulticastMessageTest.cs, updated integration tests in FirebaseMessagingTest.cs, and updated code snippets in FirebaseMessagingSnippets.cs.