From 250cf3f42dc62bddb971dc2c374ead140e6abec2 Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Mon, 8 Jan 2024 15:19:44 +0100 Subject: [PATCH 1/4] fix: setProviderAndWait does not hang on ProviderError Signed-off-by: Fabrizio Demaria --- .../dev/openfeature/sdk/OpenFeatureAPI.kt | 6 +- .../dev/openfeature/sdk/async/Extensions.kt | 17 +++- .../openfeature/sdk/events/EventHandler.kt | 9 +- .../sdk/DeveloperExperienceTests.kt | 19 +++++ .../sdk/helpers/AlwaysBrokenProvider.kt | 3 +- .../openfeature/sdk/helpers/SlowProvider.kt | 84 +++++++++++++++++++ 6 files changed, 130 insertions(+), 8 deletions(-) create mode 100644 android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt diff --git a/android/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.kt b/android/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.kt index fc247dfe..7426cf1b 100644 --- a/android/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.kt +++ b/android/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.kt @@ -11,7 +11,11 @@ object OpenFeatureAPI { fun setProvider(provider: FeatureProvider, initialContext: EvaluationContext? = null) { this@OpenFeatureAPI.provider = provider if (initialContext != null) context = initialContext - provider.initialize(context) + try { + provider.initialize(context) + } catch (e: Throwable) { + // TODO What to do here? + } } fun getProvider(): FeatureProvider? { diff --git a/android/src/main/java/dev/openfeature/sdk/async/Extensions.kt b/android/src/main/java/dev/openfeature/sdk/async/Extensions.kt index af049766..740da665 100644 --- a/android/src/main/java/dev/openfeature/sdk/async/Extensions.kt +++ b/android/src/main/java/dev/openfeature/sdk/async/Extensions.kt @@ -5,6 +5,7 @@ import dev.openfeature.sdk.FeatureProvider import dev.openfeature.sdk.OpenFeatureAPI import dev.openfeature.sdk.OpenFeatureClient import dev.openfeature.sdk.events.OpenFeatureEvents +import dev.openfeature.sdk.events.isProviderError import dev.openfeature.sdk.events.isProviderReady import dev.openfeature.sdk.events.observe import kotlinx.coroutines.CoroutineDispatcher @@ -16,6 +17,7 @@ import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.flow.take import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine +import java.lang.RuntimeException fun OpenFeatureClient.toAsync(): AsyncClient? { val provider = OpenFeatureAPI.getProvider() @@ -33,7 +35,7 @@ suspend fun OpenFeatureAPI.setProviderAndWait( initialContext: EvaluationContext? = null ) { setProvider(provider, initialContext) - provider.awaitReady(dispatcher) + provider.awaitReadyOrError(dispatcher) } internal fun FeatureProvider.observeProviderReady() = observe() @@ -43,11 +45,18 @@ internal fun FeatureProvider.observeProviderReady() = observe() + .onStart { + if (isProviderError()) { + this.emit(OpenFeatureEvents.ProviderError(RuntimeException())) // TODO Forward the correct error + } + } + inline fun OpenFeatureAPI.observeEvents(): Flow? { return getProvider()?.observe() } -suspend fun FeatureProvider.awaitReady( +suspend fun FeatureProvider.awaitReadyOrError( dispatcher: CoroutineDispatcher = Dispatchers.IO ) = suspendCancellableCoroutine { continuation -> val coroutineScope = CoroutineScope(dispatcher) @@ -60,10 +69,10 @@ suspend fun FeatureProvider.awaitReady( } coroutineScope.launch { - observe() + observeProviderError() .take(1) .collect { - continuation.resumeWith(Result.failure(it.error)) + continuation.resumeWith(Result.success(Unit)) } } diff --git a/android/src/main/java/dev/openfeature/sdk/events/EventHandler.kt b/android/src/main/java/dev/openfeature/sdk/events/EventHandler.kt index 24141460..3709a169 100644 --- a/android/src/main/java/dev/openfeature/sdk/events/EventHandler.kt +++ b/android/src/main/java/dev/openfeature/sdk/events/EventHandler.kt @@ -19,8 +19,13 @@ interface ProviderStatus { fun getProviderStatus(): OpenFeatureEvents } -fun FeatureProvider.isProviderReady(): Boolean = - getProviderStatus() == OpenFeatureEvents.ProviderReady +fun FeatureProvider.isProviderReady(): Boolean { + val providerStatus = getProviderStatus() + return providerStatus == OpenFeatureEvents.ProviderReady +} + +fun FeatureProvider.isProviderError(): Boolean = + getProviderStatus() is OpenFeatureEvents.ProviderError interface EventsPublisher { fun publish(event: OpenFeatureEvents) diff --git a/android/src/test/java/dev/openfeature/sdk/DeveloperExperienceTests.kt b/android/src/test/java/dev/openfeature/sdk/DeveloperExperienceTests.kt index 3e6c7ba6..0b473533 100644 --- a/android/src/test/java/dev/openfeature/sdk/DeveloperExperienceTests.kt +++ b/android/src/test/java/dev/openfeature/sdk/DeveloperExperienceTests.kt @@ -1,9 +1,12 @@ package dev.openfeature.sdk +import dev.openfeature.sdk.async.setProviderAndWait import dev.openfeature.sdk.exceptions.ErrorCode import dev.openfeature.sdk.helpers.AlwaysBrokenProvider import dev.openfeature.sdk.helpers.GenericSpyHookMock +import dev.openfeature.sdk.helpers.SlowProvider import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.runTest import org.junit.Assert import org.junit.Test @@ -58,4 +61,20 @@ class DeveloperExperienceTests { Assert.assertEquals("Could not find flag named: test", details.errorMessage) Assert.assertEquals(Reason.ERROR.toString(), details.reason) } + + @Test + fun testSetProviderAndWaitReady() = runTest { + val dispatcher = UnconfinedTestDispatcher() + OpenFeatureAPI.setProviderAndWait(SlowProvider(dispatcher = dispatcher), dispatcher, ImmutableContext()) + val booleanValue = OpenFeatureAPI.getClient().getBooleanValue("test", false) + Assert.assertTrue(booleanValue) + } + + @Test + fun testSetProviderAndWaitError() = runTest { + val dispatcher = UnconfinedTestDispatcher() + OpenFeatureAPI.setProviderAndWait(AlwaysBrokenProvider(), dispatcher, ImmutableContext()) + val booleanValue = OpenFeatureAPI.getClient().getBooleanValue("test", false) + Assert.assertFalse(booleanValue) + } } \ No newline at end of file diff --git a/android/src/test/java/dev/openfeature/sdk/helpers/AlwaysBrokenProvider.kt b/android/src/test/java/dev/openfeature/sdk/helpers/AlwaysBrokenProvider.kt index 27f484e4..a99e6135 100644 --- a/android/src/test/java/dev/openfeature/sdk/helpers/AlwaysBrokenProvider.kt +++ b/android/src/test/java/dev/openfeature/sdk/helpers/AlwaysBrokenProvider.kt @@ -7,6 +7,7 @@ import dev.openfeature.sdk.ProviderEvaluation import dev.openfeature.sdk.ProviderMetadata import dev.openfeature.sdk.Value import dev.openfeature.sdk.events.OpenFeatureEvents +import dev.openfeature.sdk.exceptions.OpenFeatureError import dev.openfeature.sdk.exceptions.OpenFeatureError.FlagNotFoundError import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.flow @@ -74,7 +75,7 @@ class AlwaysBrokenProvider( override fun observe(): Flow = flow { } override fun getProviderStatus(): OpenFeatureEvents = - OpenFeatureEvents.ProviderError(FlagNotFoundError("test")) + OpenFeatureEvents.ProviderError(OpenFeatureError.GeneralError("Unknown error")) class AlwaysBrokenProviderMetadata(override val name: String? = "test") : ProviderMetadata } \ No newline at end of file diff --git a/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt b/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt new file mode 100644 index 00000000..800a90c0 --- /dev/null +++ b/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt @@ -0,0 +1,84 @@ +package dev.openfeature.sdk.helpers + +import dev.openfeature.sdk.* +import dev.openfeature.sdk.events.EventHandler +import dev.openfeature.sdk.events.OpenFeatureEvents +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.launch + +class SlowProvider(override val hooks: List> = listOf(), private var dispatcher: CoroutineDispatcher) : FeatureProvider { + override val metadata: ProviderMetadata = SlowProviderMetadata("Slow provider") + private var ready = false + private var eventHandler = EventHandler(dispatcher) + override fun initialize(initialContext: EvaluationContext?) { + CoroutineScope(dispatcher).launch { + Thread.sleep(2000) // TODO Improve without sleep + ready = true + eventHandler.publish(OpenFeatureEvents.ProviderReady) + } + } + + override fun shutdown() { + // no-op + } + + override fun onContextSet( + oldContext: EvaluationContext?, + newContext: EvaluationContext + ) { + // no-op + } + + override fun getBooleanEvaluation( + key: String, + defaultValue: Boolean, + context: EvaluationContext? + ): ProviderEvaluation { + return ProviderEvaluation(!defaultValue) + } + + override fun getStringEvaluation( + key: String, + defaultValue: String, + context: EvaluationContext? + ): ProviderEvaluation { + return ProviderEvaluation(defaultValue.reversed()) + } + + override fun getIntegerEvaluation( + key: String, + defaultValue: Int, + context: EvaluationContext? + ): ProviderEvaluation { + return ProviderEvaluation(defaultValue * 100) + } + + override fun getDoubleEvaluation( + key: String, + defaultValue: Double, + context: EvaluationContext? + ): ProviderEvaluation { + return ProviderEvaluation(defaultValue * 100) + } + + override fun getObjectEvaluation( + key: String, + defaultValue: Value, + context: EvaluationContext? + ): ProviderEvaluation { + return ProviderEvaluation(Value.Null) + } + + override fun observe(): Flow = flowOf() + + override fun getProviderStatus(): OpenFeatureEvents = if (ready) { + OpenFeatureEvents.ProviderReady + } else { + OpenFeatureEvents.ProviderStale + } + + data class SlowProviderMetadata(override val name: String?) : ProviderMetadata +} \ No newline at end of file From 22d06997281d893f5505ed8fc5828121bcf3bf0d Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Mon, 8 Jan 2024 16:07:18 +0100 Subject: [PATCH 2/4] docs: Throwing on init Signed-off-by: Fabrizio Demaria --- android/src/main/java/dev/openfeature/sdk/FeatureProvider.kt | 5 +++-- android/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.kt | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/android/src/main/java/dev/openfeature/sdk/FeatureProvider.kt b/android/src/main/java/dev/openfeature/sdk/FeatureProvider.kt index 9de1a041..09523b63 100644 --- a/android/src/main/java/dev/openfeature/sdk/FeatureProvider.kt +++ b/android/src/main/java/dev/openfeature/sdk/FeatureProvider.kt @@ -8,10 +8,11 @@ interface FeatureProvider : EventObserver, ProviderStatus { val metadata: ProviderMetadata // Called by OpenFeatureAPI whenever the new Provider is registered + // This function should never throw fun initialize(initialContext: EvaluationContext?) - // called when the lifecycle of the OpenFeatureClient is over - // to release resources/threads. + // Called when the lifecycle of the OpenFeatureClient is over + // to release resources/threads fun shutdown() // Called by OpenFeatureAPI whenever a new EvaluationContext is set by the application diff --git a/android/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.kt b/android/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.kt index 7426cf1b..f487202d 100644 --- a/android/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.kt +++ b/android/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.kt @@ -14,7 +14,7 @@ object OpenFeatureAPI { try { provider.initialize(context) } catch (e: Throwable) { - // TODO What to do here? + // This is not allowed to happen } } From f31c8a239b87478dc6e65c092c1f37fb96050b59 Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Mon, 8 Jan 2024 16:16:15 +0100 Subject: [PATCH 3/4] fix: Better parsing of ProviderError Signed-off-by: Fabrizio Demaria --- .../main/java/dev/openfeature/sdk/async/Extensions.kt | 10 ++++------ .../java/dev/openfeature/sdk/events/EventHandler.kt | 9 --------- .../test/java/dev/openfeature/sdk/EventsHandlerTest.kt | 1 - .../java/dev/openfeature/sdk/helpers/SlowProvider.kt | 7 ++++++- 4 files changed, 10 insertions(+), 17 deletions(-) diff --git a/android/src/main/java/dev/openfeature/sdk/async/Extensions.kt b/android/src/main/java/dev/openfeature/sdk/async/Extensions.kt index 740da665..a583cd5f 100644 --- a/android/src/main/java/dev/openfeature/sdk/async/Extensions.kt +++ b/android/src/main/java/dev/openfeature/sdk/async/Extensions.kt @@ -5,8 +5,6 @@ import dev.openfeature.sdk.FeatureProvider import dev.openfeature.sdk.OpenFeatureAPI import dev.openfeature.sdk.OpenFeatureClient import dev.openfeature.sdk.events.OpenFeatureEvents -import dev.openfeature.sdk.events.isProviderError -import dev.openfeature.sdk.events.isProviderReady import dev.openfeature.sdk.events.observe import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope @@ -17,7 +15,6 @@ import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.flow.take import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine -import java.lang.RuntimeException fun OpenFeatureClient.toAsync(): AsyncClient? { val provider = OpenFeatureAPI.getProvider() @@ -40,15 +37,16 @@ suspend fun OpenFeatureAPI.setProviderAndWait( internal fun FeatureProvider.observeProviderReady() = observe() .onStart { - if (isProviderReady()) { + if (getProviderStatus() == OpenFeatureEvents.ProviderReady) { this.emit(OpenFeatureEvents.ProviderReady) } } internal fun FeatureProvider.observeProviderError() = observe() .onStart { - if (isProviderError()) { - this.emit(OpenFeatureEvents.ProviderError(RuntimeException())) // TODO Forward the correct error + val status = getProviderStatus() + if (status is OpenFeatureEvents.ProviderError) { + this.emit(status) } } diff --git a/android/src/main/java/dev/openfeature/sdk/events/EventHandler.kt b/android/src/main/java/dev/openfeature/sdk/events/EventHandler.kt index 3709a169..1e6ab8c4 100644 --- a/android/src/main/java/dev/openfeature/sdk/events/EventHandler.kt +++ b/android/src/main/java/dev/openfeature/sdk/events/EventHandler.kt @@ -1,6 +1,5 @@ package dev.openfeature.sdk.events -import dev.openfeature.sdk.FeatureProvider import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job @@ -19,14 +18,6 @@ interface ProviderStatus { fun getProviderStatus(): OpenFeatureEvents } -fun FeatureProvider.isProviderReady(): Boolean { - val providerStatus = getProviderStatus() - return providerStatus == OpenFeatureEvents.ProviderReady -} - -fun FeatureProvider.isProviderError(): Boolean = - getProviderStatus() is OpenFeatureEvents.ProviderError - interface EventsPublisher { fun publish(event: OpenFeatureEvents) } diff --git a/android/src/test/java/dev/openfeature/sdk/EventsHandlerTest.kt b/android/src/test/java/dev/openfeature/sdk/EventsHandlerTest.kt index 7116cd28..97b52910 100644 --- a/android/src/test/java/dev/openfeature/sdk/EventsHandlerTest.kt +++ b/android/src/test/java/dev/openfeature/sdk/EventsHandlerTest.kt @@ -4,7 +4,6 @@ import dev.openfeature.sdk.async.observeProviderReady import dev.openfeature.sdk.async.toAsync import dev.openfeature.sdk.events.EventHandler import dev.openfeature.sdk.events.OpenFeatureEvents -import dev.openfeature.sdk.events.isProviderReady import dev.openfeature.sdk.events.observe import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi diff --git a/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt b/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt index 800a90c0..ac095687 100644 --- a/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt +++ b/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt @@ -1,6 +1,11 @@ package dev.openfeature.sdk.helpers -import dev.openfeature.sdk.* +import dev.openfeature.sdk.EvaluationContext +import dev.openfeature.sdk.FeatureProvider +import dev.openfeature.sdk.Hook +import dev.openfeature.sdk.ProviderEvaluation +import dev.openfeature.sdk.ProviderMetadata +import dev.openfeature.sdk.Value import dev.openfeature.sdk.events.EventHandler import dev.openfeature.sdk.events.OpenFeatureEvents import kotlinx.coroutines.CoroutineDispatcher From 59f537fe874121148ca6d9420b7d62eaac7fc46a Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Mon, 8 Jan 2024 17:00:34 +0100 Subject: [PATCH 4/4] fix: Improve tests without using sleep Signed-off-by: Fabrizio Demaria --- .../openfeature/sdk/DeveloperExperienceTests.kt | 17 +++++++++++++---- .../dev/openfeature/sdk/helpers/SlowProvider.kt | 9 ++++++++- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/android/src/test/java/dev/openfeature/sdk/DeveloperExperienceTests.kt b/android/src/test/java/dev/openfeature/sdk/DeveloperExperienceTests.kt index 0b473533..398fc630 100644 --- a/android/src/test/java/dev/openfeature/sdk/DeveloperExperienceTests.kt +++ b/android/src/test/java/dev/openfeature/sdk/DeveloperExperienceTests.kt @@ -5,7 +5,10 @@ import dev.openfeature.sdk.exceptions.ErrorCode import dev.openfeature.sdk.helpers.AlwaysBrokenProvider import dev.openfeature.sdk.helpers.GenericSpyHookMock import dev.openfeature.sdk.helpers.SlowProvider +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.runTest import org.junit.Assert @@ -64,10 +67,16 @@ class DeveloperExperienceTests { @Test fun testSetProviderAndWaitReady() = runTest { - val dispatcher = UnconfinedTestDispatcher() - OpenFeatureAPI.setProviderAndWait(SlowProvider(dispatcher = dispatcher), dispatcher, ImmutableContext()) - val booleanValue = OpenFeatureAPI.getClient().getBooleanValue("test", false) - Assert.assertTrue(booleanValue) + val dispatcher = StandardTestDispatcher(testScheduler) + CoroutineScope(dispatcher).launch { + OpenFeatureAPI.setProviderAndWait(SlowProvider(dispatcher = dispatcher), dispatcher, ImmutableContext()) + } + testScheduler.advanceTimeBy(1) // Make sure setProviderAndWait is called + val booleanValue1 = OpenFeatureAPI.getClient().getBooleanValue("test", false) + Assert.assertFalse(booleanValue1) + testScheduler.advanceTimeBy(10000) // SlowProvider is now Ready + val booleanValue2 = OpenFeatureAPI.getClient().getBooleanValue("test", false) + Assert.assertTrue(booleanValue2) } @Test diff --git a/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt b/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt index ac095687..d391ad83 100644 --- a/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt +++ b/android/src/test/java/dev/openfeature/sdk/helpers/SlowProvider.kt @@ -8,8 +8,10 @@ import dev.openfeature.sdk.ProviderMetadata import dev.openfeature.sdk.Value import dev.openfeature.sdk.events.EventHandler import dev.openfeature.sdk.events.OpenFeatureEvents +import dev.openfeature.sdk.exceptions.OpenFeatureError import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.launch @@ -20,7 +22,7 @@ class SlowProvider(override val hooks: List> = listOf(), private var dis private var eventHandler = EventHandler(dispatcher) override fun initialize(initialContext: EvaluationContext?) { CoroutineScope(dispatcher).launch { - Thread.sleep(2000) // TODO Improve without sleep + delay(10000) ready = true eventHandler.publish(OpenFeatureEvents.ProviderReady) } @@ -42,6 +44,7 @@ class SlowProvider(override val hooks: List> = listOf(), private var dis defaultValue: Boolean, context: EvaluationContext? ): ProviderEvaluation { + if (!ready) throw OpenFeatureError.FlagNotFoundError(key) return ProviderEvaluation(!defaultValue) } @@ -50,6 +53,7 @@ class SlowProvider(override val hooks: List> = listOf(), private var dis defaultValue: String, context: EvaluationContext? ): ProviderEvaluation { + if (!ready) throw OpenFeatureError.FlagNotFoundError(key) return ProviderEvaluation(defaultValue.reversed()) } @@ -58,6 +62,7 @@ class SlowProvider(override val hooks: List> = listOf(), private var dis defaultValue: Int, context: EvaluationContext? ): ProviderEvaluation { + if (!ready) throw OpenFeatureError.FlagNotFoundError(key) return ProviderEvaluation(defaultValue * 100) } @@ -66,6 +71,7 @@ class SlowProvider(override val hooks: List> = listOf(), private var dis defaultValue: Double, context: EvaluationContext? ): ProviderEvaluation { + if (!ready) throw OpenFeatureError.FlagNotFoundError(key) return ProviderEvaluation(defaultValue * 100) } @@ -74,6 +80,7 @@ class SlowProvider(override val hooks: List> = listOf(), private var dis defaultValue: Value, context: EvaluationContext? ): ProviderEvaluation { + if (!ready) throw OpenFeatureError.FlagNotFoundError(key) return ProviderEvaluation(Value.Null) }