Add documentation, update tests and improve code style.

Author: tusharkhandelwal8

firebase-config/src/main/java/com/google/firebase/remoteconfig/CustomSignals.java

@@ -20,36 +20,60 @@
 import java.util.Map;
 
 /**
- * Helper class which handles the storage and conversion to strings of key/value pairs with
- * heterogeneous value types for custom signals.
+ * A container type to represent key/value pairs of heterogeneous types to be set as custom signals
+ * in {@link FirebaseRemoteConfig#setCustomSignals}.
  */
 public class CustomSignals {
-
   final Map<String, String> customSignals;
 
+  /** Builder for constructing {@link CustomSignals} instances. */
   public static class Builder {
-    // Holds the converted pairs of custom keys and values.
     private Map<String, String> customSignals = new HashMap<String, String>();
 
-    // Methods to accept keys and values and convert values to strings.
+    /**
+     * Adds a custom signal with a value that can be a string or null to the builder.
+     *
+     * @param key The key for the custom signal.
+     * @param value The string value associated with the key. Can be null.
+     * @return This Builder instance to allow chaining of method calls.
+     */
     @NonNull
     public Builder put(@NonNull String key, @Nullable String value) {
       customSignals.put(key, value);
       return this;
     }
 
+    /**
+     * Adds a custom signal with a long value to the builder.
+     *
+     * @param key The key for the custom signal.
+     * @param value The long value for the custom signal.
+     * @return This Builder instance to allow chaining of method calls.
+     */
     @NonNull
     public Builder put(@NonNull String key, long value) {
       customSignals.put(key, Long.toString(value));
       return this;
     }
 
+    /**
+     * Adds a custom signal with a double value to the builder.
+     *
+     * @param key The key for the custom signal.
+     * @param value The double value for the custom signal.
+     * @return This Builder instance to allow chaining of method calls.
+     */
     @NonNull
     public Builder put(@NonNull String key, double value) {
       customSignals.put(key, Double.toString(value));
       return this;
     }
 
+    /**
+     * Creates a {@link CustomSignals} instance with the added custom signals.
+     *
+     * @return The constructed {@link CustomSignals} instance.
+     */
     @NonNull
     public CustomSignals build() {
       return new CustomSignals(this);

firebase-config/src/main/java/com/google/firebase/remoteconfig/FirebaseRemoteConfig.java

@@ -655,8 +655,10 @@ private Task<Void> setDefaultsWithStringsMapAsync(Map<String, String> defaultsSt
   /**
    * Asynchronously changes the custom signals for this {@link FirebaseRemoteConfig} instance.
    *
-   * <p>The {@code customSignals} parameter should be an instance of {@link CustomSignals}, which
-   * enforces the allowed types for custom signal values (String, Long or Double).
+   * <p>The {@code customSignals} parameter should be an instance of {@link CustomSignals}.
+   *
+   * <p>Custom signals are subject to limits on the size of key/value pairs and the total
+   * number of signals. Any calls that exceed these limits will be discarded.
    *
    * @param customSignals A dictionary of keys and the values of the custom signals to be set for
    *                         the app instance

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigSharedPrefsClient.java

@@ -37,6 +37,7 @@
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
+import java.util.Objects;
 import org.json.JSONException;
 import org.json.JSONObject;
 
@@ -272,6 +273,8 @@ public void setCustomSignals(Map<String, String> newCustomSignals) {
     synchronized (customSignalsLock) {
       // Retrieve existing custom signals
       Map<String, String> existingCustomSignals = getCustomSignals();
+      // Tracks whether the custom signals have been modified.
+      boolean modified = false;
 
       for (Map.Entry<String, String> entry : newCustomSignals.entrySet()) {
         String key = entry.getKey();
@@ -291,14 +294,14 @@ public void setCustomSignals(Map<String, String> newCustomSignals) {
         // Merge new signals with existing ones, overwriting existing keys.
         // Also, remove entries where the new value is null.
         if (value != null) {
-          existingCustomSignals.put(key, value);
+          modified |= !Objects.equals(existingCustomSignals.put(key, value), value);
         } else {
-          existingCustomSignals.remove(key);
+          modified |= existingCustomSignals.remove(key) != null;
         }
       }
 
       // Check if the map has actually changed and the size limit
-      if (existingCustomSignals.equals(getCustomSignals())) {
+      if (!modified) {
         return;
       }
       if (existingCustomSignals.size() > CUSTOM_SIGNALS_MAX_COUNT) {

firebase-config/src/test/java/com/google/firebase/remoteconfig/CustomSignalsTest.java

@@ -14,69 +14,73 @@
 
 package com.google.firebase.remoteconfig;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
+import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.collect.ImmutableMap;
 import java.util.HashMap;
 import java.util.Map;
 import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.robolectric.RobolectricTestRunner;
-import org.robolectric.annotation.Config;
 
 /** Unit tests for the {@link CustomSignals}.*/
-@RunWith(RobolectricTestRunner.class)
-@Config(manifest = Config.NONE)
 public class CustomSignalsTest {
   @Test
   public void testCustomSignals_builderPutString() {
     CustomSignals customSignals =
         new CustomSignals.Builder().put("key1", "value1").put("key2", "value2").build();
     Map<String, String> expectedSignals = ImmutableMap.of("key1", "value1", "key2", "value2");
-    assertEquals(expectedSignals, customSignals.customSignals);
+    assertThat(customSignals.customSignals).isEqualTo(expectedSignals);
   }
 
   @Test
   public void testCustomSignals_builderPutLong() {
     CustomSignals customSignals =
         new CustomSignals.Builder().put("key1", 123L).put("key2", 456L).build();
     Map<String, String> expectedSignals = ImmutableMap.of("key1", "123", "key2", "456");
-    assertEquals(expectedSignals, customSignals.customSignals);
+    assertThat(customSignals.customSignals).isEqualTo(expectedSignals);
   }
 
   @Test
   public void testCustomSignals_builderPutDouble() {
     CustomSignals customSignals =
         new CustomSignals.Builder().put("key1", 12.34).put("key2", 56.78).build();
     Map<String, String> expectedSignals = ImmutableMap.of("key1", "12.34", "key2", "56.78");
-    assertEquals(expectedSignals, customSignals.customSignals);
+    assertThat(customSignals.customSignals).isEqualTo(expectedSignals);
   }
 
   @Test
-  public void testCustomSignals_builderPutMixedTypes() {
+  public void testCustomSignals_builderPutNullValue() {
+    CustomSignals customSignals = new CustomSignals.Builder().put("key1", null).build();
+    Map<String, String> expectedSignals = new HashMap<>();
+    expectedSignals.put("key1", null);
+    assertThat(customSignals.customSignals).isEqualTo(expectedSignals);
+  }
+
+  @Test
+  public void testCustomSignals_builderPutDuplicateKeys() {
     CustomSignals customSignals =
         new CustomSignals.Builder()
             .put("key1", "value1")
-            .put("key2", 123L)
-            .put("key3", 45.67)
+            .put("key1", "value2")
+            .put("key1", "value3")
             .build();
-    Map<String, String> expectedSignals =
-        ImmutableMap.of("key1", "value1", "key2", "123", "key3", "45.67");
-    assertEquals(expectedSignals, customSignals.customSignals);
+    Map<String, String> expectedSignals = ImmutableMap.of("key1", "value3");
+    assertThat(customSignals.customSignals).isEqualTo(expectedSignals);
   }
 
   @Test
-  public void testCustomSignals_builderPutNullValue() {
-    CustomSignals customSignals = new CustomSignals.Builder().put("key1", null).build();
+  public void testCustomSignals_builderPutMixedTypes() {
+    CustomSignals customSignals =
+        new CustomSignals.Builder()
+            .put("key1", "value1")
+            .put("key2", 123L)
+            .put("key3", 45.67)
+            .put("key4", null)
+            .build();
     Map<String, String> expectedSignals = new HashMap<>();
-    expectedSignals.put("key1", null);
-    assertEquals(expectedSignals, customSignals.customSignals);
-  }
-
-  @Test
-  public void testCustomSignals_builderEmpty() {
-    CustomSignals customSignals = new CustomSignals.Builder().build();
-    assertTrue(customSignals.customSignals.isEmpty());
+    expectedSignals.put("key1", "value1");
+    expectedSignals.put("key2", "123");
+    expectedSignals.put("key3", "45.67");
+    expectedSignals.put("key4", null);
+    assertThat(customSignals.customSignals).isEqualTo(expectedSignals);
   }
 }

firebase-config/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigTest.java

@@ -1667,6 +1667,7 @@ public void setCustomSignals_succeeds_and_calls_sharedPrefsClient() {
             .put("key1", "value1")
             .put("key2", 123L)
             .put("key3", 12.34)
+            .put("key4", null)
             .build();
 
     Task<Void> setterTask = frc.setCustomSignals(customSignals);

firebase-config/src/test/java/com/google/firebase/remoteconfig/RemoteConfigTests.kt

@@ -17,7 +17,6 @@
 package com.google.firebase.remoteconfig
 
 import androidx.test.core.app.ApplicationProvider
-import com.google.common.collect.ImmutableMap
 import com.google.common.truth.Truth.assertThat
 import com.google.common.util.concurrent.MoreExecutors
 import com.google.firebase.Firebase
@@ -150,58 +149,17 @@ class ConfigTests : BaseTestCase() {
   }
 
   @Test
-  fun `Custom Signals builder put string`() {
-    val customSignals = customSignals {
-      put("key1", "value1")
-      put("key2", "value2")
-    }
-    val expectedSignals = ImmutableMap.of("key1", "value1", "key2", "value2")
-    assertThat(customSignals.customSignals).isEqualTo(expectedSignals)
-  }
-
-  @Test
-  fun `Custom Signals builder put long`() {
-    val customSignals = customSignals {
-      put("key1", 123L)
-      put("key2", 456L)
-    }
-    val expectedSignals = ImmutableMap.of("key1", "123", "key2", "456")
-    assertThat(customSignals.customSignals).isEqualTo(expectedSignals)
-  }
-
-  @Test
-  fun `Custom Signals builder put double`() {
-    val customSignals = customSignals {
-      put("key1", 12.34)
-      put("key2", 56.78)
-    }
-    val expectedSignals = ImmutableMap.of("key1", "12.34", "key2", "56.78")
-    assertThat(customSignals.customSignals).isEqualTo(expectedSignals)
-  }
-
-  @Test
-  fun `Custom Signals builder put mixed types`() {
+  fun `Custom Signals builder support multiple types`() {
     val customSignals = customSignals {
       put("key1", "value1")
       put("key2", 123L)
       put("key3", 45.67)
+      put("key4", null)
     }
-    val expectedSignals = ImmutableMap.of("key1", "value1", "key2", "123", "key3", "45.67")
+    val expectedSignals =
+      mapOf("key1" to "value1", "key2" to "123", "key3" to "45.67", "key4" to null)
     assertThat(customSignals.customSignals).isEqualTo(expectedSignals)
   }
-
-  @Test
-  fun `Custom Signals builder put null value`() {
-    val customSignals = customSignals { put("key1", null) }
-    val expectedSignals = mapOf("key1" to null)
-    assertThat(customSignals.customSignals).isEqualTo(expectedSignals)
-  }
-
-  @Test
-  fun `Custom Signals empty builder`() {
-    val customSignals = customSignals {}
-    assertThat(customSignals.customSignals).isEmpty()
-  }
 }
 
 @RunWith(RobolectricTestRunner::class)