feat(fcm): Enable fid and deprecate token for Send API#1211
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) as message targets in the FCM SDK, deprecating registration token fields in favor of FIDs across the Message and MulticastMessage classes. Feedback indicates that the current implementation of MulticastMessage.getMessageList() fails to preserve the relative order of tokens and FIDs, which violates the API contract requiring response order to match the input order. Additionally, the class-level documentation for MulticastMessage needs to be updated to reflect the new combined limit of 500 for tokens and FIDs.
| List<Message> getMessageList() { | ||
| ImmutableList.Builder<Message> messages = ImmutableList.builder(); | ||
|
|
||
| if (!this.tokens.isEmpty()) { | ||
| Message.Builder tokenBuilder = getMetadataBuilder(); | ||
| for (String token : this.tokens) { | ||
| messages.add(tokenBuilder.setToken(token).build()); | ||
| } | ||
| } | ||
|
|
||
| if (!this.fids.isEmpty()) { | ||
| Message.Builder fidBuilder = getMetadataBuilder(); | ||
| for (String fid : this.fids) { | ||
| messages.add(fidBuilder.setFid(fid).build()); | ||
| } | ||
| } | ||
|
|
||
| return messages.build(); | ||
| } |
There was a problem hiding this comment.
The current implementation of getMessageList() does not preserve the relative order of registration tokens and FIDs when both are present in a MulticastMessage. The class documentation and the sendEachForMulticast API contract specify that the order of responses in the BatchResponse must match the order in which targets were added to the message. However, the current logic groups all tokens first and then all FIDs.
To fix this, the MulticastMessage.Builder should maintain a single unified list of targets to track the exact sequence of additions, rather than storing them in separate tokens and fids builders.
|
|
||
| @Deprecated | ||
| private final List<String> tokens; | ||
| private final List<String> fids; |
There was a problem hiding this comment.
The class-level Javadoc (specifically lines 33-35 and 41-44) should be updated to include references to FIDs. Currently, it only mentions "registration tokens" and states that the 500 limit applies to tokens, whereas the implementation now applies this limit to the combined total of tokens and FIDs.
| assertMessage(messages.get(0), "token1"); | ||
| assertMessage(messages.get(1), "token2"); | ||
| assertMessageFid(messages.get(2), "fid1"); | ||
| assertMessageFid(messages.get(3), "fid2"); |
There was a problem hiding this comment.
This test case confirms that the implementation does not preserve the relative order of targets. Even though tokens and FIDs are added in an interleaved manner (token1, fid1, token2, fid2), the test expects all tokens to appear before all FIDs. This behavior contradicts the API documentation regarding order preservation. The test should be updated to assert the actual addition order once the implementation is fixed.
1 participant
This change deprecates
token(inMessage) andtokens(inMulticastMessage) as message targets, transitioning support tofid(Firebase Installation ID) andfidsas the primary targets instead.