feat(fcm): Enable fid and deprecate token for Send API#525
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Firebase Installation IDs (FIDs) in Firebase Cloud Messaging (FCM) by adding the Fid property to Message and the Fids property to MulticastMessage, while deprecating the older Token and Tokens properties. It updates validation, copying, and serialization logic to support these new fields, adds comprehensive unit and integration tests, and suppresses deprecation warnings where necessary. The review feedback suggests improving consistency in exception messages by capitalizing property names (Tokens and Fids) and ensuring proper punctuation.
There was a problem hiding this comment.
Shall we update the comments for SendEachForMulticastAsync to mention the message response order in BatchResponse when both Tokens and FIDs are set in MulticastMessage?
There was a problem hiding this comment.
Sure! Updated comments for both SendEachForMulticastAsync and SendMulticastAsync
There was a problem hiding this comment.
Could we update this test file to include some tests using fid messages?
There was a problem hiding this comment.
MessageTest.cs is already handling the payload serialization/deserialization logic for FIDs. FirebaseMessagingClientTest.cs is responsible for testing the HTTP transport layer. It typically uses a generic Topic or Token payload just as a dummy placeholder to verify that the HTTP request is built correctly. Since we already have full FID coverage in MessageTest.cs and the FirebaseMessagingTest.cs integration tests, it seems a bit redundant to add some tests using fid messages in FirebaseMessagingClientTest.cs. But please let me know if you'd still prefer I add a dummy FID payload test there for completeness!
There was a problem hiding this comment.
If we prefer not to add new tests, it would also be nice to simply replace the Token payload with the FID payload as a dummy placeholder. Since the Token payload is deprecated, this would also avoid the #pragma warning. Additionally, this covers testing the integration with the FID payload.
There was a problem hiding this comment.
Sounds good! I have replaced the Token payload with the FID one. Thanks for the detailed review!
| #pragma warning disable CS0618 | ||
| Token = deviceToken, | ||
| #pragma warning restore CS0618 | ||
| Notification = new Notification() |
There was a problem hiding this comment.
Can you add extend the code snippet to show the new usage with Fids?
There was a problem hiding this comment.
Sure! Code snippets for the new usage with Fids are added.
| { | ||
| return @"{ | ||
| ""error"": { | ||
| ""status"": ""INVALID_ARGUMENT"", |
There was a problem hiding this comment.
What is the reason for removing the INVALID_ARGUMENT error code? I think it is good to verify that a 400 Bad Request with INVALID_ARGUMENT error converts to MessagingErrorCode.Unregistered (for tokens) unless the recent fix changes that for tokens as well.
There was a problem hiding this comment.
I was doing a complete replacement from Token to Fid in this file but you're right, it's good to verify the bad request for tokens. I have added them back.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Firebase Installation IDs (FIDs) in Firebase Cloud Messaging (FCM) by adding Fid to Message and Fids to MulticastMessage, while deprecating the older Token and Tokens properties. It updates validation, serialization, and multicast sending logic to support up to 500 combined targets, alongside adding corresponding tests and snippets. However, several critical syntax errors were identified in FirebaseMessagingClientTest.cs where trailing commas were mistakenly appended to #pragma warning restore CS0618 preprocessor directives, which will cause compilation failures.
Co-authored-by: OrlandriaC-G <112568492+OrlandriaH-G@users.noreply.github.com>
Co-authored-by: OrlandriaC-G <112568492+OrlandriaH-G@users.noreply.github.com>
Co-authored-by: OrlandriaC-G <112568492+OrlandriaH-G@users.noreply.github.com>
4 participants
This change deprecates
Token(inMessage) andTokens(inMulticastMessage) as message targets, transitioning support toFid(Firebase Installation ID) andFidsas the primary targets instead.Legacy token tests are wrapped in Method-Level #pragma warning disable CS0618 to prevent compiler warnings and build crash and #pragma wrappers are explicitly maintained for snippet documentation code to allow compiling flawlessly.