From 2b29438a1494e6803dfa689e296aa312837edd45 Mon Sep 17 00:00:00 2001 From: mariiapanasetskaia Date: Mon, 31 Aug 2026 14:32:42 +0200 Subject: [PATCH] put the member in the Unleash context before the first fetch --- .../feature-flags/build.gradle.kts | 10 + .../android/featureflags/FakeUnleash.kt | 83 +++++++ .../featureflags/HedvigUnleashClientTest.kt | 209 ++++++++++++++++++ .../featureflags/HedvigUnleashClient.kt | 142 ++++++++---- .../android/featureflags/FeatureManager.kt | 5 +- 5 files changed, 411 insertions(+), 38 deletions(-) create mode 100644 app/featureflags/feature-flags/src/androidHostTest/kotlin/com/hedvig/android/featureflags/FakeUnleash.kt create mode 100644 app/featureflags/feature-flags/src/androidHostTest/kotlin/com/hedvig/android/featureflags/HedvigUnleashClientTest.kt diff --git a/app/featureflags/feature-flags/build.gradle.kts b/app/featureflags/feature-flags/build.gradle.kts index 8783e0cce6..a8ea56e335 100644 --- a/app/featureflags/feature-flags/build.gradle.kts +++ b/app/featureflags/feature-flags/build.gradle.kts @@ -10,6 +10,9 @@ hedvig { } kotlin { + androidLibrary { + withHostTest {} + } sourceSets { commonMain.dependencies { implementation(libs.coroutines.core) @@ -21,5 +24,12 @@ kotlin { implementation(projects.coreBuildConstants) implementation(projects.coreCommonPublic) } + getByName("androidHostTest").dependencies { + implementation(libs.assertK) + implementation(libs.coroutines.test) + implementation(libs.junit) + implementation(libs.turbine) + implementation(projects.loggingTest) + } } } diff --git a/app/featureflags/feature-flags/src/androidHostTest/kotlin/com/hedvig/android/featureflags/FakeUnleash.kt b/app/featureflags/feature-flags/src/androidHostTest/kotlin/com/hedvig/android/featureflags/FakeUnleash.kt new file mode 100644 index 0000000000..361e39c0c6 --- /dev/null +++ b/app/featureflags/feature-flags/src/androidHostTest/kotlin/com/hedvig/android/featureflags/FakeUnleash.kt @@ -0,0 +1,83 @@ +package com.hedvig.android.featureflags + +import io.getunleash.android.Unleash +import io.getunleash.android.data.Toggle +import io.getunleash.android.data.UnleashContext +import io.getunleash.android.data.Variant +import io.getunleash.android.disabledVariant +import io.getunleash.android.events.UnleashListener +import io.getunleash.android.events.UnleashReadyListener +import io.getunleash.android.events.UnleashStateListener +import java.io.File + +/** + * Stands in for the Unleash SDK, modelling the two pieces of its behaviour the client depends on: + * a toggle set that only holds the toggles a fetch actually returned, and a readiness flag that the + * SDK raises off the first non-empty set regardless of which context produced it. + */ +internal class FakeUnleash : Unleash { + val contexts = mutableListOf() + var started = false + private set + + private var toggles: Map = emptyMap() + private var ready = false + private val listeners = mutableListOf() + + /** + * Delivers the toggle set a fetch returned. Only enabled toggles reach the client in the frontend + * API's response, so a flag left out of [enabledToggles] is one the backend did not send. + */ + fun completeFetch(enabledToggles: Set) { + toggles = enabledToggles.associateWith { true } + val becameReady = !ready && enabledToggles.isNotEmpty() + ready = ready || enabledToggles.isNotEmpty() + listeners.filterIsInstance().forEach { it.onStateChanged() } + if (becameReady) { + listeners.filterIsInstance().forEach { it.onReady() } + } + } + + override fun isEnabled(toggleName: String): Boolean = toggles[toggleName] ?: false + + @Deprecated("Use isEnabled(toggleName: String) instead.", ReplaceWith("isEnabled(toggleName)")) + override fun isEnabled(toggleName: String, defaultValue: Boolean): Boolean = toggles[toggleName] ?: defaultValue + + override fun isReady(): Boolean = ready + + override fun setContextAsync(context: UnleashContext) { + contexts.add(context) + } + + override fun setContext(context: UnleashContext) = setContextAsync(context) + + override fun setContextWithTimeout(context: UnleashContext, timeout: Long) = setContextAsync(context) + + override fun addUnleashEventListener(listener: UnleashListener) { + listeners.add(listener) + } + + override fun removeUnleashEventListener(listener: UnleashListener) { + listeners.remove(listener) + } + + override fun start(eventListeners: List, bootstrapFile: File?, bootstrap: List) { + started = true + eventListeners.forEach(::addUnleashEventListener) + } + + override fun getVariant(toggleName: String): Variant = disabledVariant + + @Deprecated("Use getVariant(toggleName: String) instead.", ReplaceWith("getVariant(toggleName)")) + override fun getVariant(toggleName: String, defaultValue: Variant): Variant = defaultValue + + override fun refreshTogglesNow() = Unit + + override fun refreshTogglesNowAsync() = Unit + + override fun sendMetricsNow() = Unit + + override fun sendMetricsNowAsync() = Unit + + override fun close() = Unit +} diff --git a/app/featureflags/feature-flags/src/androidHostTest/kotlin/com/hedvig/android/featureflags/HedvigUnleashClientTest.kt b/app/featureflags/feature-flags/src/androidHostTest/kotlin/com/hedvig/android/featureflags/HedvigUnleashClientTest.kt new file mode 100644 index 0000000000..52d8c72347 --- /dev/null +++ b/app/featureflags/feature-flags/src/androidHostTest/kotlin/com/hedvig/android/featureflags/HedvigUnleashClientTest.kt @@ -0,0 +1,209 @@ +package com.hedvig.android.featureflags + +import assertk.assertThat +import assertk.assertions.isEqualTo +import assertk.assertions.isFalse +import assertk.assertions.isNull +import assertk.assertions.isTrue +import com.hedvig.android.featureflags.flags.Feature +import com.hedvig.android.logger.TestLogcatLoggingRule +import java.io.IOException +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.cancel +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.currentTime +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Rule +import org.junit.Test + +@OptIn(ExperimentalCoroutinesApi::class) +internal class HedvigUnleashClientTest { + @get:Rule + val testLogcatLogger = TestLogcatLoggingRule() + + private val clientScopes = mutableListOf() + + @After + fun cancelClientScopes() = clientScopes.forEach(CoroutineScope::cancel) + + /** + * The client gets a scope of its own, sharing the test scheduler, rather than the test's: its + * member id collection never completes, so the test coroutine would never finish waiting for it. + * A background scope would not do either, since virtual time is not advanced for background work + * alone and the client waits out a timeout when no member id arrives. + */ + private fun TestScope.client(unleash: FakeUnleash, memberIds: Flow): HedvigUnleashClient { + val scope = CoroutineScope(StandardTestDispatcher(testScheduler)) + clientScopes.add(scope) + return HedvigUnleashClient( + client = unleash, + appVersionName = "14.4.6", + coroutineScope = scope, + memberIds = memberIds, + ) + } + + @Test + fun `while no toggle set has been delivered a flag reads its never-fetched default`() = runTest { + val unleash = FakeUnleash() + val client = client(unleash, MutableStateFlow("member-1")) + runCurrent() + + assertThat(client.valueOf(Feature.DISABLE_ANALYTICS)).isTrue() + assertThat(client.valueOf(Feature.DISABLE_RESUMING_ONGOING_SHOP_SESSIONS)).isTrue() + } + + @Test + fun `while no toggle set has been delivered a flag without a never-fetched default reads false`() = runTest { + val unleash = FakeUnleash() + val client = client(unleash, MutableStateFlow("member-1")) + runCurrent() + + assertThat(client.valueOf(Feature.ENABLE_CLAIM_INTENT_RESUME)).isFalse() + } + + @Test + fun `an empty toggle set leaves the client unready, so flags keep their never-fetched defaults`() = runTest { + val unleash = FakeUnleash() + val client = client(unleash, MutableStateFlow("member-1")) + runCurrent() + + unleash.completeFetch(enabledToggles = emptySet()) + + assertThat(client.valueOf(Feature.DISABLE_ANALYTICS)).isTrue() + } + + @Test + fun `a flag left out of a delivered toggle set reads false, not its never-fetched default`() = runTest { + val unleash = FakeUnleash() + val client = client(unleash, MutableStateFlow("member-1")) + runCurrent() + + unleash.completeFetch(enabledToggles = setOf("disable_onboarding")) + + assertThat(client.valueOf(Feature.DISABLE_ANALYTICS)).isFalse() + assertThat(client.valueOf(Feature.DISABLE_RESUMING_ONGOING_SHOP_SESSIONS)).isFalse() + assertThat(client.valueOf(Feature.DISABLE_ONBOARDING)).isTrue() + } + + @Test + fun `the first fetch carries the member, so member-sticky strategies resolve for them`() = runTest { + val unleash = FakeUnleash() + client(unleash, MutableStateFlow("member-1")) + runCurrent() + + val contextAtStart = unleash.contexts.last() + assertThat(contextAtStart.userId).isEqualTo("member-1") + assertThat(contextAtStart.properties["memberId"]).isEqualTo("member-1") + assertThat(unleash.started).isTrue() + } + + @Test + fun `a logged out member starts with no member in context, without waiting out the timeout`() = runTest { + val unleash = FakeUnleash() + client(unleash, MutableStateFlow(null)) + runCurrent() + + assertThat(unleash.started).isTrue() + assertThat(unleash.contexts.last().userId).isNull() + assertThat(unleash.contexts.last().properties["memberId"]).isNull() + assertThat(currentTime).isEqualTo(0L) + } + + @Test + fun `a member id that never arrives still starts Unleash once the wait expires`() = runTest { + val unleash = FakeUnleash() + client(unleash, MutableSharedFlow()) + advanceUntilIdle() + + assertThat(unleash.started).isTrue() + } + + @Test + fun `a member id that cannot be read still starts Unleash`() = runTest { + val unleash = FakeUnleash() + client(unleash, flow { throw IOException("Token store unreadable") }) + advanceUntilIdle() + + assertThat(unleash.started).isTrue() + assertThat(unleash.contexts.last().userId).isNull() + } + + @Test + fun `awaitReady returns once the toggle set for the member in context lands`() = runTest { + val unleash = FakeUnleash() + val client = client(unleash, MutableStateFlow("member-1")) + runCurrent() + + var returned = false + backgroundScope.launch { + client.awaitReady() + returned = true + } + runCurrent() + assertThat(returned).isFalse() + + unleash.completeFetch(enabledToggles = setOf("disable_analytics")) + runCurrent() + + assertThat(returned).isTrue() + } + + @Test + fun `awaitReady does not return on a toggle set fetched before the member logged in`() = runTest { + val unleash = FakeUnleash() + val memberIds = MutableStateFlow(null) + val client = client(unleash, memberIds) + runCurrent() + // Logged out, so the set fetched anonymously is the right answer and readiness is satisfied. + unleash.completeFetch(enabledToggles = setOf("disable_onboarding")) + runCurrent() + + memberIds.value = "member-1" + runCurrent() + // The member-sticky kill switch is absent from the set in hand purely because the fetch that + // produced it named no member, which is what awaitReady must not let a caller act on. + assertThat(client.valueOf(Feature.DISABLE_ANALYTICS)).isFalse() + + var returned = false + backgroundScope.launch { + client.awaitReady() + returned = true + } + runCurrent() + assertThat(returned).isFalse() + + unleash.completeFetch(enabledToggles = setOf("disable_onboarding", "disable_analytics")) + runCurrent() + + assertThat(returned).isTrue() + assertThat(client.valueOf(Feature.DISABLE_ANALYTICS)).isTrue() + } + + @Test + fun `awaitReady returns immediately for a logged out member once a toggle set lands`() = runTest { + val unleash = FakeUnleash() + val client = client(unleash, MutableStateFlow(null)) + runCurrent() + unleash.completeFetch(enabledToggles = setOf("disable_onboarding")) + + var returned = false + backgroundScope.launch { + client.awaitReady() + returned = true + } + runCurrent() + + assertThat(returned).isTrue() + } +} diff --git a/app/featureflags/feature-flags/src/androidMain/kotlin/com/hedvig/android/featureflags/HedvigUnleashClient.kt b/app/featureflags/feature-flags/src/androidMain/kotlin/com/hedvig/android/featureflags/HedvigUnleashClient.kt index 615106d9e6..3fde211640 100644 --- a/app/featureflags/feature-flags/src/androidMain/kotlin/com/hedvig/android/featureflags/HedvigUnleashClient.kt +++ b/app/featureflags/feature-flags/src/androidMain/kotlin/com/hedvig/android/featureflags/HedvigUnleashClient.kt @@ -4,20 +4,26 @@ import android.content.Context import com.hedvig.android.auth.MemberIdService import com.hedvig.android.featureflags.flags.Feature import com.hedvig.android.featureflags.flags.unleashKey +import com.hedvig.android.logger.LogPriority import com.hedvig.android.logger.logcat import io.getunleash.android.DefaultUnleash +import io.getunleash.android.Unleash import io.getunleash.android.UnleashConfig import io.getunleash.android.data.UnleashContext import io.getunleash.android.events.UnleashReadyListener import io.getunleash.android.events.UnleashStateListener import kotlin.coroutines.resume +import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.channels.awaitClose import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.callbackFlow -import kotlinx.coroutines.flow.collectLatest +import kotlinx.coroutines.flow.catch +import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.withTimeoutOrNull private const val PRODUCTION_CLIENT_KEY = "*:production.21d6af57ae16320fde3a3caf024162db19cc33bf600ab7439c865c20" private const val DEVELOPMENT_CLIENT_KEY = "*:development.f2455340ac9d599b5816fa879d079f21dd0eb03e4315130deb5377b6" @@ -37,22 +43,62 @@ private val neverFetchedDefaults: Map = mapOf( Feature.DISABLE_RESUMING_ONGOING_SHOP_SESSIONS to true, ) -class HedvigUnleashClient( - private val androidContext: Context, - private val isProduction: Boolean, +/** + * How long the first member id is waited for before starting Unleash without one. The token store + * emits its current contents as soon as it is read, and a logged out member's is an immediate null, + * so no launch spends this waiting: it exists only so a read that never completes cannot stop + * Unleash from starting. A member id arriving afterwards is applied by the ongoing collection. + */ +private val INITIAL_MEMBER_ID_TIMEOUT = 2.seconds + +private fun createUnleashConfig(isProduction: Boolean): UnleashConfig { + val clientKey = if (isProduction) { + PRODUCTION_CLIENT_KEY + } else { + DEVELOPMENT_CLIENT_KEY + } + + return UnleashConfig.newBuilder(APP_NAME) + .proxyUrl(UNLEASH_URL) + .clientKey(clientKey) + .pollingStrategy.interval(2000) + .metricsStrategy.interval(2000) + .build() +} + +class HedvigUnleashClient internal constructor( + private val client: Unleash, private val appVersionName: String, coroutineScope: CoroutineScope, - private val memberIdService: MemberIdService, + private val memberIds: Flow, ) { - private val client = DefaultUnleash( - androidContext = androidContext, - unleashConfig = createConfig(), - unleashContext = createContext( - appVersion = appVersionName, - memberId = null, + constructor( + androidContext: Context, + isProduction: Boolean, + appVersionName: String, + coroutineScope: CoroutineScope, + memberIdService: MemberIdService, + ) : this( + client = DefaultUnleash( + androidContext = androidContext, + unleashConfig = createUnleashConfig(isProduction), ), + appVersionName = appVersionName, + coroutineScope = coroutineScope, + memberIds = memberIdService.getMemberId(), ) + private var contextInEffect: UnleashContext? = null + + /** + * Whether the toggles in hand were fetched for [contextInEffect]. Unleash resolves strategies + * server-side against the context the fetch carried, so a set fetched before the member was known + * resolves a member-sticky strategy as if there were no member, which yields false even at a 100% + * rollout. The SDK's own readiness only means some toggles arrived, and the anonymous set the app + * fetches at startup satisfies it. + */ + private val togglesMatchContext = MutableStateFlow(false) + /** * Emits whenever the toggle state changes for any reason, which includes the local backup being * restored and not just a network fetch, so a flag read while offline still updates once the @@ -94,17 +140,27 @@ class HedvigUnleashClient( } } + /** + * Suspends until flag values are available for the current member: Unleash reports isReady(), and + * the toggles in hand were fetched for the context naming that member rather than an earlier + * anonymous one (see [togglesMatchContext]). Never completes while the app has never fetched and + * cannot reach Unleash, so callers must impose a timeout. + */ + suspend fun awaitReady() { + awaitClientReady() + togglesMatchContext.first { it } + } + /** * Suspends until Unleash reports isReady(), which it sets on the first non-empty toggle set from - * either the on-disk backup or a network fetch, returning immediately if it already has. Never - * completes while the app has never fetched and cannot reach Unleash, so callers must impose a timeout. + * either the on-disk backup or a network fetch, returning immediately if it already has. * * Any toggle state seeded through the SDK's `bootstrap` parameter would also satisfy this, since * readiness is just "the toggle cache became non-empty". Seeding it would additionally stop the * on-disk backup from ever loading, which the SDK only attempts while not yet ready. Defaults for * the never-fetched window therefore live in [neverFetchedDefaults], outside the SDK's cache. */ - suspend fun awaitReady() { + private suspend fun awaitClientReady() { if (client.isReady()) return suspendCancellableCoroutine { continuation -> val listener = object : UnleashReadyListener { @@ -121,36 +177,43 @@ class HedvigUnleashClient( } init { + client.addUnleashEventListener( + object : UnleashStateListener { + override fun onStateChanged() { + togglesMatchContext.value = true + } + }, + ) coroutineScope.launch { - memberIdService.getMemberId().collectLatest { memberId: String? -> - client.setContextAsync( - createContext( - appVersion = appVersionName, - memberId = memberId, - ), - ) + // Reading the member id must not be able to keep Unleash from starting: without a start there + // is no fetch, and every flag would spend the session on its never-fetched default. + val memberIdsOrNone = memberIds.catch { throwable -> + logcat(LogPriority.ERROR, throwable) { "Failed to read the member id for the Unleash context" } + emit(null) } + // Put the member in the context before the first fetch rather than switching to them after + // it, so that fetch already resolves member-sticky strategies for them, and so the on-disk + // backup, which the SDK keys by context, still matches on the next launch. + applyContext(withTimeoutOrNull(INITIAL_MEMBER_ID_TIMEOUT) { memberIdsOrNone.first() }) + client.start() + memberIdsOrNone.collect { memberId: String? -> applyContext(memberId) } } - client.start() } - private fun createConfig(): UnleashConfig { - val clientKey = if (isProduction) { - PRODUCTION_CLIENT_KEY - } else { - DEVELOPMENT_CLIENT_KEY - } - - return UnleashConfig.newBuilder(APP_NAME) - .proxyUrl(UNLEASH_URL) - .clientKey(clientKey) - .pollingStrategy.interval(2000) - .metricsStrategy.interval(2000) - .build() + /** + * Points the client at [memberId]'s context, marking the toggles in hand as no longer matching it + * until the fetch the change triggers lands. Called only from the single collecting coroutine. + */ + private fun applyContext(memberId: String?) { + val context = createContext(appVersion = appVersionName, memberId = memberId) + if (context == contextInEffect) return + contextInEffect = context + togglesMatchContext.value = false + client.setContextAsync(context) } private fun createContext(appVersion: String, memberId: String?): UnleashContext { - return UnleashContext.newBuilder() + val builder = UnleashContext.newBuilder() .properties( buildMap { put("appVersion", appVersion) @@ -161,6 +224,11 @@ class HedvigUnleashClient( } }.toMutableMap(), ) - .build() + if (memberId != null) { + // The property backs constraints and `memberId` stickiness; userId is the standard field the + // `default` stickiness resolves, without which a partial rollout re-dices on every poll. + builder.userId(memberId) + } + return builder.build() } } diff --git a/app/featureflags/feature-flags/src/commonMain/kotlin/com/hedvig/android/featureflags/FeatureManager.kt b/app/featureflags/feature-flags/src/commonMain/kotlin/com/hedvig/android/featureflags/FeatureManager.kt index a44136ca40..0c4b6e5dbc 100644 --- a/app/featureflags/feature-flags/src/commonMain/kotlin/com/hedvig/android/featureflags/FeatureManager.kt +++ b/app/featureflags/feature-flags/src/commonMain/kotlin/com/hedvig/android/featureflags/FeatureManager.kt @@ -9,11 +9,14 @@ interface FeatureManager { fun isFeatureEnabled(feature: Feature): Flow /** - * Suspends until flag values are available for the current session, whether freshly fetched from the + * Suspends until flag values are available for the current member, whether freshly fetched from the * backend or restored from the last fetch's local cache. A decision that must honor the flag, for * example a kill switch gating a whole flow, should await this and treat a failure to complete as "no * value available yet". Until the app has ever reached the backend there is nothing to restore either, * so this never completes; callers must impose their own timeout. + * + * Values are resolved for the member the app knows about at fetch time, so a flag whose strategy is + * sticky on the member resolves for them rather than anonymously once this completes. */ suspend fun awaitReady() }