WIP feat(fcm): Add support for AndroidConfigV2 - #537
Conversation
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.
|
/gemini review |
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.
…sing in AndroidConfigV2
This change introduces FCM Android V2 messaging configuration support by adding
AndroidConfigV2,AndroidNotificationV2,AndroidRemoteNotification, andAndroidBackgroundSyncMessagemodels toFirebaseAdmin.Messaging. The legacyAndroidproperty onMessageandMulticastMessagehas been marked as obsolete in favor ofAndroidV2. Validation enforces mutual exclusivity betweenAndroidandAndroidV2onMessage, as well as requiring exactly one ofRemoteNotificationorBackgroundSyncto be specified onAndroidConfigV2.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.csandMulticastMessageTest.cs, updated integration tests inFirebaseMessagingTest.cs, and updated code snippets inFirebaseMessagingSnippets.cs.