From 526adce6035773ed64be2dc6f0f874fb3ef6a5d0 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Fri, 24 Jan 2020 15:29:03 -0500 Subject: [PATCH] Add support for sticky and hot-swappable feature flags. --- .../securesms/ApplicationContext.java | 1 + .../jobs/RemoteConfigRefreshJob.java | 4 +- .../securesms/util/FeatureFlags.java | 170 +++++++++--- .../securesms/util/FeatureFlagsTest.java | 251 ++++++++++++++++++ 4 files changed, 389 insertions(+), 37 deletions(-) create mode 100644 app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java diff --git a/app/src/main/java/org/thoughtcrime/securesms/ApplicationContext.java b/app/src/main/java/org/thoughtcrime/securesms/ApplicationContext.java index eba1b00587..0d07d12559 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/ApplicationContext.java +++ b/app/src/main/java/org/thoughtcrime/securesms/ApplicationContext.java @@ -150,6 +150,7 @@ public class ApplicationContext extends MultiDexApplication implements DefaultLi public void onStart(@NonNull LifecycleOwner owner) { isAppVisible = true; Log.i(TAG, "App is now visible."); + FeatureFlags.refresh(); ApplicationDependencies.getRecipientCache().warmUp(); executePendingContactSync(); KeyCachingService.onAppForegrounded(this); diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/RemoteConfigRefreshJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/RemoteConfigRefreshJob.java index 5b3a5f07c9..50cf9e42cc 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/RemoteConfigRefreshJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/RemoteConfigRefreshJob.java @@ -7,10 +7,8 @@ import org.thoughtcrime.securesms.jobmanager.Data; import org.thoughtcrime.securesms.jobmanager.Job; import org.thoughtcrime.securesms.keyvalue.SignalStore; import org.thoughtcrime.securesms.util.FeatureFlags; -import org.thoughtcrime.securesms.util.TextSecurePreferences; import org.whispersystems.signalservice.api.push.exceptions.PushNetworkException; -import java.util.HashMap; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -44,7 +42,7 @@ public class RemoteConfigRefreshJob extends BaseJob { @Override protected void onRun() throws Exception { Map config = ApplicationDependencies.getSignalServiceAccountManager().getRemoteConfig(); - FeatureFlags.updateDiskCache(config); + FeatureFlags.update(config); SignalStore.setRemoteConfigLastFetchTime(System.currentTimeMillis()); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java index dc4d6df1ef..3e1f53b1b6 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java +++ b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java @@ -3,6 +3,9 @@ package org.thoughtcrime.securesms.util; import android.text.TextUtils; import androidx.annotation.NonNull; +import androidx.annotation.VisibleForTesting; + +import com.annimon.stream.Stream; import org.json.JSONException; import org.json.JSONObject; @@ -12,9 +15,12 @@ import org.thoughtcrime.securesms.keyvalue.SignalStore; import org.thoughtcrime.securesms.logging.Log; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.Map; +import java.util.Set; import java.util.TreeMap; +import java.util.TreeSet; import java.util.concurrent.TimeUnit; /** @@ -28,6 +34,10 @@ import java.util.concurrent.TimeUnit; * - If you would like to force a value for testing, place an entry in {@link #FORCED_VALUES}. When * launching a feature that is planned to be updated via a remote config, do not forget to * remove the entry! + * + * Other interesting things you can do: + * - Make a flag {@link #HOT_SWAPPABLE} + * - Make a flag {@link #STICKY} */ public final class FeatureFlags { @@ -56,73 +66,143 @@ public final class FeatureFlags { put(STORAGE_SERVICE, false); }}; - private static final Map REMOTE_VALUES = new HashMap<>(); + /** + * By default, flags are only updated once at app start. This is to ensure that values don't + * change within an app session, simplifying logic. However, given that this can delay how often + * a flag is updated, you can put a flag in here to mark it as 'hot swappable'. Flags in this set + * will be updated arbitrarily at runtime. This will make values more responsive, but also places + * more burden on the reader to ensure that the app experience remains consistent. + */ + private static final Set HOT_SWAPPABLE = new TreeSet() {{ + }}; + + /** + * Flags in this set will stay true forever once they receive a true value from a remote config. + */ + private static final Set STICKY = new HashSet() {{ + }}; + + private static final Map REMOTE_VALUES = new TreeMap<>(); private FeatureFlags() {} - public static void init() { - scheduleFetchIfNecessary(); + public static synchronized void init() { REMOTE_VALUES.putAll(parseStoredConfig()); + Log.i(TAG, "init() " + REMOTE_VALUES.toString()); } - public static void updateDiskCache(@NonNull Map config) { - try { - JSONObject filtered = new JSONObject(); + public static synchronized void refresh() { + long timeSinceLastFetch = System.currentTimeMillis() - SignalStore.getRemoteConfigLastFetchTime(); - for (Map.Entry entry : config.entrySet()) { - if (entry.getKey().startsWith(PREFIX)) { - filtered.put(entry.getKey(), (boolean) entry.getValue()); - } - } - - SignalStore.setRemoteConfig(filtered.toString()); - } catch (JSONException e) { - throw new AssertionError(e); + if (timeSinceLastFetch > FETCH_INTERVAL) { + Log.i(TAG, "Scheduling remote config refresh."); + ApplicationDependencies.getJobManager().add(new RemoteConfigRefreshJob()); + } else { + Log.i(TAG, "Skipping remote config refresh. Refreshed " + timeSinceLastFetch + " ms ago."); } } + public static synchronized void update(@NonNull Map config) { + Map memory = REMOTE_VALUES; + Map disk = parseStoredConfig(); + UpdateResult result = updateInternal(config, memory, disk, HOT_SWAPPABLE, STICKY); + + SignalStore.setRemoteConfig(mapToJson(result.getDisk()).toString()); + REMOTE_VALUES.clear(); + REMOTE_VALUES.putAll(result.getMemory()); + + Log.i(TAG, "[Memory] Before: " + memory.toString()); + Log.i(TAG, "[Memory] After : " + result.getMemory().toString()); + Log.i(TAG, "[Disk] Before: " + disk.toString()); + Log.i(TAG, "[Disk] After : " + result.getDisk().toString()); + } + /** UUID-related stuff that shouldn't be activated until the user-facing launch. */ - public static boolean uuids() { + public static synchronized boolean uuids() { return getValue(UUIDS, false); } /** Favoring profile names when displaying contacts. */ - public static boolean profileDisplay() { + public static synchronized boolean profileDisplay() { return getValue(PROFILE_DISPLAY, false); } /** MessageRequest stuff */ - public static boolean messageRequests() { + public static synchronized boolean messageRequests() { return getValue(MESSAGE_REQUESTS, false); } /** Creating usernames, sending messages by username. Requires {@link #uuids()}. */ - public static boolean usernames() { + public static synchronized boolean usernames() { boolean value = getValue(USERNAMES, false); if (value && !uuids()) throw new MissingFlagRequirementError(); return value; } /** Storage service. */ - public static boolean storageService() { + public static synchronized boolean storageService() { return getValue(STORAGE_SERVICE, false); } /** Send support for reactions. */ - public static boolean reactionSending() { + public static synchronized boolean reactionSending() { return getValue(REACTION_SENDING, false); } /** Only for rendering debug info. */ - public static @NonNull Map getRemoteValues() { + public static synchronized @NonNull Map getRemoteValues() { return new TreeMap<>(REMOTE_VALUES); } /** Only for rendering debug info. */ - public static @NonNull Map getForcedValues() { + public static synchronized @NonNull Map getForcedValues() { return new TreeMap<>(FORCED_VALUES); } + @VisibleForTesting + static @NonNull UpdateResult updateInternal(@NonNull Map remote, + @NonNull Map localMemory, + @NonNull Map localDisk, + @NonNull Set hotSwap, + @NonNull Set sticky) + { + Map newMemory = new TreeMap<>(localMemory); + Map newDisk = new TreeMap<>(localDisk); + + Set allKeys = new HashSet<>(); + allKeys.addAll(remote.keySet()); + allKeys.addAll(localDisk.keySet()); + allKeys.addAll(localMemory.keySet()); + + Stream.of(allKeys) + .filter(k -> k.startsWith(PREFIX)) + .forEach(key -> { + Boolean remoteValue = remote.get(key); + Boolean diskValue = localDisk.get(key); + Boolean newValue = remoteValue; + + if (sticky.contains(key)) { + newValue = diskValue == Boolean.TRUE ? Boolean.TRUE : newValue; + } + + if (newValue != null) { + newDisk.put(key, newValue); + } else { + newDisk.remove(key); + } + + if (hotSwap.contains(key)) { + if (newValue != null) { + newMemory.put(key, newValue); + } else { + newMemory.remove(key); + } + } + }); + + return new UpdateResult(newMemory, newDisk); + } + private static @NonNull String generateKey(@NonNull String key) { return PREFIX + key; } @@ -141,17 +221,6 @@ public final class FeatureFlags { return defaultValue; } - private static void scheduleFetchIfNecessary() { - long timeSinceLastFetch = System.currentTimeMillis() - SignalStore.getRemoteConfigLastFetchTime(); - - if (timeSinceLastFetch > FETCH_INTERVAL) { - Log.i(TAG, "Scheduling remote config refresh."); - ApplicationDependencies.getJobManager().add(new RemoteConfigRefreshJob()); - } else { - Log.i(TAG, "Skipping remote config refresh. Refreshed " + timeSinceLastFetch + " ms ago."); - } - } - private static Map parseStoredConfig() { Map parsed = new HashMap<>(); String stored = SignalStore.getRemoteConfig(); @@ -177,6 +246,39 @@ public final class FeatureFlags { return parsed; } + private static JSONObject mapToJson(@NonNull Map map) { + try { + JSONObject json = new JSONObject(); + + for (Map.Entry entry : map.entrySet()) { + json.put(entry.getKey(), (boolean) entry.getValue()); + } + + return json; + } catch (JSONException e) { + throw new AssertionError(e); + } + } + private static final class MissingFlagRequirementError extends Error { } + + @VisibleForTesting + static final class UpdateResult { + private final Map memory; + private final Map disk; + + UpdateResult(@NonNull Map memory, @NonNull Map disk) { + this.memory = memory; + this.disk = disk; + } + + public @NonNull Map getMemory() { + return memory; + } + + public @NonNull Map getDisk() { + return disk; + } + } } diff --git a/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java b/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java new file mode 100644 index 0000000000..d3761329ec --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java @@ -0,0 +1,251 @@ +package org.thoughtcrime.securesms.util; + +import org.junit.Test; +import org.thoughtcrime.securesms.util.FeatureFlags.UpdateResult; + +import java.util.Arrays; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +import static org.junit.Assert.assertEquals; + +public class FeatureFlagsTest { + + private static final String A = key("a"); + private static final String B = key("b"); + + @Test + public void updateInternal_newValue_ignoreMissingPrefix() { + UpdateResult result = FeatureFlags.updateInternal(mapOf("noprefix", true), + mapOf(), + mapOf(), + setOf(), + setOf()); + + assertEquals(mapOf(), result.getMemory()); + assertEquals(mapOf(), result.getDisk()); + } + + @Test + public void updateInternal_newValue() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true), + mapOf(), + mapOf(), + setOf(), + setOf()); + + assertEquals(mapOf(), result.getMemory()); + assertEquals(mapOf(A, true), result.getDisk()); + } + + @Test + public void updateInternal_newValue_hotSwap() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true), + mapOf(), + mapOf(), + setOf(A), + setOf()); + + assertEquals(mapOf(A, true), result.getMemory()); + assertEquals(mapOf(A, true), result.getDisk()); + } + + @Test + public void updateInternal_newValue_sticky() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true), + mapOf(), + mapOf(), + setOf(), + setOf(A)); + + assertEquals(mapOf(), result.getMemory()); + assertEquals(mapOf(A, true), result.getDisk()); + } + + @Test + public void updateInternal_newValue_hotSwap_sticky() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true), + mapOf(), + mapOf(), + setOf(A), + setOf(A)); + + assertEquals(mapOf(A, true), result.getMemory()); + assertEquals(mapOf(A, true), result.getDisk()); + } + + @Test + public void updateInternal_replaceValue() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true), + mapOf(A, false), + mapOf(A, false), + setOf(), + setOf()); + + assertEquals(mapOf(A, false), result.getMemory()); + assertEquals(mapOf(A, true), result.getDisk()); + } + + @Test + public void updateInternal_replaceValue_hotSwap() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true), + mapOf(A, false), + mapOf(A, false), + setOf(A), + setOf()); + + assertEquals(mapOf(A, true), result.getMemory()); + assertEquals(mapOf(A, true), result.getDisk()); + } + + @Test + public void updateInternal_replaceValue_hotSwap_stickyChange() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true), + mapOf(A, false), + mapOf(A, false), + setOf(A), + setOf(A)); + + assertEquals(mapOf(A, true), result.getMemory()); + assertEquals(mapOf(A, true), result.getDisk()); + } + + @Test + public void updateInternal_replaceValue_hotSwap_stickyIgnore_memoryAndDisk() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, false), + mapOf(A, true), + mapOf(A, true), + setOf(A), + setOf(A)); + + assertEquals(mapOf(A, true), result.getMemory()); + assertEquals(mapOf(A, true), result.getDisk()); + } + + @Test + public void updateInternal_removeValue() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(), + mapOf(A, true), + mapOf(A, true), + setOf(), + setOf()); + + assertEquals(mapOf(A, true), result.getMemory()); + assertEquals(mapOf(), result.getDisk()); + } + + @Test + public void updateInternal_removeValue_hotSwap() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(), + mapOf(A, true), + mapOf(A, true), + setOf(A), + setOf()); + + assertEquals(mapOf(), result.getMemory()); + assertEquals(mapOf(), result.getDisk()); + } + + @Test + public void updateInternal_removeValue_stickyAlreadyEnabled() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(), + mapOf(A, true), + mapOf(A, true), + setOf(), + setOf(A)); + + assertEquals(mapOf(A, true), result.getMemory()); + assertEquals(mapOf(A, true), result.getDisk()); + } + + @Test + public void updateInternal_removeValue_stickyNotEnabled() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(), + mapOf(A, false), + mapOf(A, false), + setOf(), + setOf(A)); + + assertEquals(mapOf(A, false), result.getMemory()); + assertEquals(mapOf(), result.getDisk()); + } + + @Test + public void updateInternal_removeValue_hotSwap_stickyAlreadyEnabled() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(), + mapOf(A, true), + mapOf(A, true), + setOf(A), + setOf(A)); + + assertEquals(mapOf(A, true), result.getMemory()); + assertEquals(mapOf(A, true), result.getDisk()); + } + + @Test + public void updateInternal_removeValue_hotSwap_stickyNotEnabled() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(), + mapOf(A, false), + mapOf(A, false), + setOf(A), + setOf(A)); + + assertEquals(mapOf(), result.getMemory()); + assertEquals(mapOf(), result.getDisk()); + } + + @Test + public void updateInternal_twoNewValues() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true, + B, false), + mapOf(), + mapOf(), + setOf(), + setOf()); + + assertEquals(mapOf(), result.getMemory()); + assertEquals(mapOf(A, true, B, false), result.getDisk()); + } + + @Test + public void updateInternal_replaceOneOfTwoValues() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true, + B, false), + mapOf(A, true, + B, true), + mapOf(A, true, + B, true), + setOf(), + setOf()); + + assertEquals(mapOf(A, true, B, true), result.getMemory()); + assertEquals(mapOf(A, true, B, false), result.getDisk()); + } + + private static String key(String s) { + return "android." + s; + } + + private static Set setOf(V... values) { + return new HashSet<>(Arrays.asList(values)); + } + + private static Map mapOf() { + return new HashMap<>(); + } + + private static Map mapOf(K k, V v) { + return new HashMap() {{ + put(k, v); + }}; + } + + private static Map mapOf(K k1, V v1, K k2, V v2) { + return new HashMap() {{ + put(k1, v1); + put(k2, v2); + }}; + } +}