Code clarity on MulticastMessage class

[NunuM]

Environment

• Firebase SDK version: 6.8.1
• Firebase Product: messaging

Problem

The class com.google.firebase.messaging.MulticastMessage represents the concept of one message for multiple targets. The companion class com.google.firebase.messaging.MulticastMessage.Builder accepts as an argument a java.util.List of targets (addAllTokens method). The data type associated (List) allows repeated elements, in other words, we can build a MulticastMessage instance, that has a list of repeated targets.

I presume that in server side this will be unmarshalled to a Set data type. On MulticastMessage constructor can I change it to eliminate repeated elements and make a PR. Or this is meant to be like this?

Thanks.

Relevant Code:

// Internal data-structure for 'tokens' attribute on MulticastMessage class
final ImmutableList.Builder<String> objectBuilder = ImmutableList.builder();

objectBuilder.add("one");
objectBuilder.add("one");

final ImmutableList<String> build = objectBuilder.build();

System.out.println(build); // [one, one]

[hiranya911]

There's no server-side equivalent of MulticastMessage. That object gets broken down into a List<Message> in the SDK itself.

Use of List in this class is intentional since we want to retain the order of tokens so they can be later correlated with the order of responses in BatchResponse. Plus we don't want to be opinionated about whether to allow duplicate tokens or not. There might be obscure use cases (e.g. in testing) where you want to send a message to the same device repeatedly. And on top of that changing List to Set in the public API would be a breaking change.

Having said that it might make sense for addAllTokens() to accept a Collection instead of List. This makes it easier for developers to pass in a Set if they want to enforce uniqueness:

// Developers will have to be careful to use a Set implementation with deterministic iteration order.
Set<string> uniqueTokens = new LinkedHashSet();
uniqueTokens.add("one");
uniqueTokens.add("two");

MulticastMessage.builder()
  .addAllTokens(uniqueTokens)
  .build(); 

[NunuM]

Thanks for your clear and quick response.

I will take your design advice to ensure uniqueness and iteration order.

Thank you.

[hiranya911]

You're welcome @NunuM. Just to make it clear, the existing API in MulticastMessage.Builder doesn't allow passing a Collection. My point was that it's something we can change the existing API into.

[NunuM]

@hiranya911 as you mentioned, I have changed the argument data type of MulticastMessage.Builder#addAllTokens method from List to Collection. Do you agree with the diff?

Thank you.

diff --git a/src/main/java/com/google/firebase/messaging/MulticastMessage.java b/src/main/java/com/google/firebase/messaging/MulticastMessage.java
index 32c9365..2c4c1c3 100644
--- a/src/main/java/com/google/firebase/messaging/MulticastMessage.java
+++ b/src/main/java/com/google/firebase/messaging/MulticastMessage.java
@@ -22,6 +22,8 @@ import com.google.api.client.util.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.firebase.internal.NonNull;
+
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -116,10 +118,10 @@ public class MulticastMessage {
      * Adds a list of tokens to which the message should be sent. Up to 100 tokens can be
      * specified on a single instance of {@link MulticastMessage}.
      *
-     * @param tokens List of Firebase device registration tokens.
+     * @param tokens Collection of Firebase device registration tokens.
      * @return This builder.
      */
-    public Builder addAllTokens(@NonNull List<String> tokens) {
+    public Builder addAllTokens(@NonNull Collection<String> tokens) {
       this.tokens.addAll(tokens);
       return this;
     }

[hiranya911]

Yes, that seems correct.