Get parameter value for rollout without side effects. (#5710)

The `getString(String parameterKey)` method calls listeners as a
side-effect, which isn't the behavior we want when `activate()` is
called and Rollouts are published.

This duplicates the `getString` method without the side-effects (as well
as a helper method to access the cache). Alternatively we could
duplicate this method _within_ `ConfigGetParameterHandler`, but it'd
have to be `public` which implies other callers should use it (they
shouldn't - there's no other use case besides Rollouts).

Google internal: b/324275156

firebase-config/CHANGELOG.md

@@ -1,4 +1,6 @@
 # Unreleased
+* [fixed] Fixed an issue that could cause [remote_config] personalizations to be logged early in
+  specific cases.
 
 
 # 21.6.1

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

@@ -173,7 +173,7 @@ public synchronized FirebaseRemoteConfig get(String namespace) {
     }
 
     RolloutsStateSubscriptionsHandler rolloutsStateSubscriptionsHandler =
-        getRolloutsStateSubscriptionsHandler(activatedCacheClient, getHandler);
+        getRolloutsStateSubscriptionsHandler(activatedCacheClient, defaultsCacheClient);
 
     return get(
         firebaseApp,
@@ -321,10 +321,9 @@ private static Personalization getPersonalization(
   }
 
   private RolloutsStateSubscriptionsHandler getRolloutsStateSubscriptionsHandler(
-      ConfigCacheClient activatedConfigsCache,
-      ConfigGetParameterHandler configGetParameterHandler) {
+      ConfigCacheClient activatedConfigsCache, ConfigCacheClient defaultConfigsCache) {
     RolloutsStateFactory rolloutsStateFactory =
-        RolloutsStateFactory.create(configGetParameterHandler);
+        RolloutsStateFactory.create(activatedConfigsCache, defaultConfigsCache);
 
     return new RolloutsStateSubscriptionsHandler(
         activatedConfigsCache, rolloutsStateFactory, executor);

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

@@ -16,14 +16,17 @@
 
 package com.google.firebase.remoteconfig.internal.rollouts;
 
+import static com.google.firebase.remoteconfig.FirebaseRemoteConfig.DEFAULT_VALUE_FOR_STRING;
 import static com.google.firebase.remoteconfig.FirebaseRemoteConfig.TAG;
 import static com.google.firebase.remoteconfig.internal.ConfigContainer.ROLLOUT_METADATA_AFFECTED_KEYS;
 import static com.google.firebase.remoteconfig.internal.ConfigContainer.ROLLOUT_METADATA_ID;
 import static com.google.firebase.remoteconfig.internal.ConfigContainer.ROLLOUT_METADATA_VARIANT_ID;
 
 import android.util.Log;
 import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 import com.google.firebase.remoteconfig.FirebaseRemoteConfigClientException;
+import com.google.firebase.remoteconfig.internal.ConfigCacheClient;
 import com.google.firebase.remoteconfig.internal.ConfigContainer;
 import com.google.firebase.remoteconfig.internal.ConfigGetParameterHandler;
 import com.google.firebase.remoteconfig.interop.rollouts.RolloutAssignment;
@@ -35,10 +38,13 @@
 import org.json.JSONObject;
 
 public class RolloutsStateFactory {
-  ConfigGetParameterHandler getParameterHandler;
+  ConfigCacheClient activatedConfigsCache;
+  ConfigCacheClient defaultConfigsCache;
 
-  RolloutsStateFactory(ConfigGetParameterHandler getParameterHandler) {
-    this.getParameterHandler = getParameterHandler;
+  RolloutsStateFactory(
+      ConfigCacheClient activatedConfigsCache, ConfigCacheClient defaultConfigsCache) {
+    this.activatedConfigsCache = activatedConfigsCache;
+    this.defaultConfigsCache = defaultConfigsCache;
   }
 
   @NonNull
@@ -66,7 +72,7 @@ RolloutsState getActiveRolloutsState(@NonNull ConfigContainer configContainer)
 
         // Fallback to empty string if (for some reason) there's no affected parameter key.
         String parameterKey = affectedParameterKeys.optString(0, "");
-        String parameterValue = getParameterHandler.getString(parameterKey);
+        String parameterValue = getParameterValue(parameterKey);
 
         rolloutAssignments.add(
             RolloutAssignment.builder()
@@ -85,9 +91,58 @@ RolloutsState getActiveRolloutsState(@NonNull ConfigContainer configContainer)
     return RolloutsState.create(rolloutAssignments);
   }
 
+  /**
+   * Returns the parameter value of the given parameter key as a {@link String}.
+   *
+   * <p>The logic in {@link ConfigGetParameterHandler#getString} is duplicated here without the side
+   * effect of calling listeners (ex. logging Personalizations in Google Analytics).
+   *
+   * @param key A Firebase Remote Config parameter key.
+   */
+  @NonNull
+  private String getParameterValue(@NonNull String key) {
+    String activatedString = getStringFromCache(activatedConfigsCache, key);
+    if (activatedString != null) {
+      return activatedString;
+    }
+
+    String defaultsString = getStringFromCache(defaultConfigsCache, key);
+    if (defaultsString != null) {
+      return defaultsString;
+    }
+
+    return DEFAULT_VALUE_FOR_STRING;
+  }
+
+  /**
+   * Returns the FRC parameter value for the given key in the given cache as a {@link String}, or
+   * {@code null} if the key does not exist in the cache.
+   *
+   * <p>This method is duplicated from {@link ConfigGetParameterHandler}. Future work might move the
+   * ConfigCacheClient helpers into {@link ConfigCacheClient}.
+   *
+   * @param cacheClient the cache client the parameter is stored in.
+   * @param key the FRC parameter key.
+   */
+  @Nullable
+  private static String getStringFromCache(
+      @NonNull ConfigCacheClient cacheClient, @NonNull String key) {
+    ConfigContainer cachedContainer = cacheClient.getBlocking();
+    if (cachedContainer == null) {
+      return null;
+    }
+
+    try {
+      return cachedContainer.getConfigs().getString(key);
+    } catch (JSONException ignored) {
+      return null;
+    }
+  }
+
   @NonNull
   public static RolloutsStateFactory create(
-      @NonNull ConfigGetParameterHandler configGetParameterHandler) {
-    return new RolloutsStateFactory(configGetParameterHandler);
+      @NonNull ConfigCacheClient activatedConfigsCache,
+      @NonNull ConfigCacheClient defaultConfigsCache) {
+    return new RolloutsStateFactory(activatedConfigsCache, defaultConfigsCache);
   }
 }

firebase-config/src/test/java/com/google/firebase/remoteconfig/internal/ConfigGetParameterHandlerTest.java

@@ -21,8 +21,12 @@
 import static com.google.firebase.remoteconfig.FirebaseRemoteConfig.DEFAULT_VALUE_FOR_LONG;
 import static com.google.firebase.remoteconfig.FirebaseRemoteConfig.DEFAULT_VALUE_FOR_STRING;
 import static com.google.firebase.remoteconfig.internal.ConfigGetParameterHandler.FRC_BYTE_ARRAY_ENCODING;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import com.google.android.gms.common.util.BiConsumer;
 import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.android.gms.tasks.Tasks;
 import com.google.common.collect.ImmutableMap;
@@ -77,6 +81,7 @@ public class ConfigGetParameterHandlerTest {
 
   @Mock private ConfigCacheClient mockActivatedCache;
   @Mock private ConfigCacheClient mockDefaultsCache;
+  @Mock private BiConsumer<String, ConfigContainer> mockListener;
 
   private ConfigGetParameterHandler getHandler;
 
@@ -172,6 +177,19 @@ public void getString_activatedStillLoadingFromDisk_blocksAndReturnsActivatedVal
     assertThat(stringValue).isEqualTo(ACTIVATED_STRING_VALUE);
   }
 
+  @Test
+  public void getString_activatedKeyExists_callsListeners() throws Exception {
+    loadActivatedCacheWithMap(ImmutableMap.of(STRING_KEY, ACTIVATED_STRING_VALUE));
+    loadCacheWithConfig(mockDefaultsCache, /*container=*/ null);
+
+    getHandler.addListener(this.mockListener);
+
+    String stringValue = getHandler.getString(STRING_KEY);
+
+    assertThat(stringValue).isEqualTo(ACTIVATED_STRING_VALUE);
+    verify(this.mockListener).accept(eq(STRING_KEY), any(ConfigContainer.class));
+  }
+
   @Test
   public void getBoolean_activatedAndDefaultsKeysExist_returnsActivatedValue() throws Exception {
     loadActivatedCacheWithMap(ImmutableMap.of(BOOLEAN_KEY, ACTIVATED_BOOLEAN_STRING_VALUE));

firebase-config/src/test/java/com/google/firebase/remoteconfig/internal/rollouts/RolloutsStateFactoryTest.java

@@ -22,10 +22,12 @@
 
 import com.google.common.collect.ImmutableSet;
 import com.google.firebase.remoteconfig.FirebaseRemoteConfigClientException;
+import com.google.firebase.remoteconfig.internal.ConfigCacheClient;
 import com.google.firebase.remoteconfig.internal.ConfigContainer;
-import com.google.firebase.remoteconfig.internal.ConfigGetParameterHandler;
 import com.google.firebase.remoteconfig.interop.rollouts.RolloutAssignment;
 import com.google.firebase.remoteconfig.interop.rollouts.RolloutsState;
+import java.util.HashMap;
+import java.util.Map;
 import org.json.JSONArray;
 import org.junit.Before;
 import org.junit.Test;
@@ -36,7 +38,9 @@
 
 @RunWith(RobolectricTestRunner.class)
 public class RolloutsStateFactoryTest {
-  @Mock ConfigGetParameterHandler mockConfigGetParameterHandler;
+  @Mock ConfigCacheClient activatedConfigsCache;
+
+  @Mock ConfigCacheClient defaultConfigsCache;
 
   private static final String PARAMETER_KEY = "my_feature";
   private static final String PARAMETER_VALUE = "true";
@@ -60,8 +64,12 @@ public class RolloutsStateFactoryTest {
   public void setUp() throws Exception {
     MockitoAnnotations.initMocks(this);
 
+    Map<String, String> configsMap = new HashMap<>();
+    configsMap.put(PARAMETER_KEY, PARAMETER_VALUE);
+
     configContainerWithRollout =
         ConfigContainer.newBuilder()
+            .replaceConfigsWith(configsMap)
             .withRolloutMetadata(
                 new JSONArray(
                     "["
@@ -76,7 +84,7 @@ public void setUp() throws Exception {
             .build();
     configContainerWithoutRollout = ConfigContainer.newBuilder().build();
 
-    factory = new RolloutsStateFactory(mockConfigGetParameterHandler);
+    factory = new RolloutsStateFactory(activatedConfigsCache, defaultConfigsCache);
   }
 
   @Test
@@ -88,7 +96,19 @@ public void getActiveRolloutsState_noRollouts_returnsEmptyState() throws Excepti
 
   @Test
   public void getActiveRolloutsState_hasRolloutsMetadata_populatesRolloutsState() throws Exception {
-    when(mockConfigGetParameterHandler.getString(PARAMETER_KEY)).thenReturn(PARAMETER_VALUE);
+    when(activatedConfigsCache.getBlocking()).thenReturn(configContainerWithRollout);
+
+    RolloutsState actual = factory.getActiveRolloutsState(configContainerWithRollout);
+
+    assertThat(actual).isEqualTo(rolloutsState);
+  }
+
+  @Test
+  public void
+      getActiveRolloutsState_hasRolloutsMetadata_noActivatedCacheValue_populatesRolloutsStateFromDefault()
+          throws Exception {
+    when(activatedConfigsCache.getBlocking()).thenReturn(configContainerWithoutRollout);
+    when(defaultConfigsCache.getBlocking()).thenReturn(configContainerWithRollout);
 
     RolloutsState actual = factory.getActiveRolloutsState(configContainerWithRollout);
 
@@ -97,7 +117,7 @@ public void getActiveRolloutsState_hasRolloutsMetadata_populatesRolloutsState()
 
   @Test
   public void getActiveRolloutsState_rolloutsMetadataMultipleKeys_keepsFirstKey() throws Exception {
-    when(mockConfigGetParameterHandler.getString(PARAMETER_KEY)).thenReturn(PARAMETER_VALUE);
+    when(activatedConfigsCache.getBlocking()).thenReturn(configContainerWithRollout);
 
     // Add a key to affectedParameterKeys in configContainerWithRollout
     ConfigContainer configContainerWithRolloutWithMultipleKeys =
@@ -124,8 +144,8 @@ public void getActiveRolloutsState_rolloutsMetadataMultipleKeys_keepsFirstKey()
 
   @Test
   public void getActiveRolloutsState_jsonException_throwsRemoteConfigException() throws Exception {
-    when(mockConfigGetParameterHandler.getString(PARAMETER_KEY)).thenReturn(PARAMETER_VALUE);
-    JSONArray rolloutMetadatamMissingVariantId =
+    when(activatedConfigsCache.getBlocking()).thenReturn(configContainerWithRollout);
+    JSONArray rolloutMetadataMissingVariantId =
         new JSONArray(
             "["
                 + "{"
@@ -140,7 +160,7 @@ public void getActiveRolloutsState_jsonException_throwsRemoteConfigException() t
         () ->
             factory.getActiveRolloutsState(
                 ConfigContainer.newBuilder()
-                    .withRolloutMetadata(rolloutMetadatamMissingVariantId)
+                    .withRolloutMetadata(rolloutMetadataMissingVariantId)
                     .build()));
   }
 }