PR comments; remove redundant comments, fix test definitions, move strategy into MultiProvider

Signed-off-by: penguindan <[email protected]>

Author: PenguinDan

kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/FirstMatchStrategy.kt

@@ -8,24 +8,15 @@ import dev.openfeature.kotlin.sdk.exceptions.ErrorCode
 import dev.openfeature.kotlin.sdk.exceptions.OpenFeatureError
 
 /**
- * Return the first result returned by a provider. Skip providers that indicate they had no value due to FLAG_NOT_FOUND.
- * In all other cases, use the value returned by the provider. If any provider returns an error result other than
- * FLAG_NOT_FOUND, the whole evaluation should error and "bubble up" the individual provider's error in the result.
+ * A [MultiProvider.Strategy] that returns the first result returned by a [FeatureProvider].
  *
- * As soon as a value is returned by a provider, the rest of the operation should short-circuit and not call the
- * rest of the providers.
+ * It skips providers that indicate they had no value due to [ErrorCode.FLAG_NOT_FOUND].
+ * In all other cases, it uses the value returned by the provider.
+ *
+ * If any provider returns an error result other than [ErrorCode.FLAG_NOT_FOUND], the whole evaluation
+ * returns the provider's error.
  */
-class FirstMatchStrategy : Strategy {
-    /**
-     * Evaluates providers in sequence until finding one that has knowledge of the flag.
-     *
-     * @param providers List of providers to evaluate in order
-     * @param key The feature flag key to look up
-     * @param defaultValue Value to return if no provider knows about the flag
-     * @param evaluationContext Optional context for evaluation
-     * @param flagEval The specific evaluation method to call on each provider
-     * @return ProviderEvaluation with the first match or default value
-     */
+class FirstMatchStrategy : MultiProvider.Strategy {
     override fun <T> evaluate(
         providers: List<FeatureProvider>,
         key: String,

kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/FirstSuccessfulStrategy.kt

@@ -6,23 +6,12 @@ import dev.openfeature.kotlin.sdk.ProviderEvaluation
 import dev.openfeature.kotlin.sdk.exceptions.OpenFeatureError
 
 /**
- * Similar to "First Match", except that errors from evaluated providers do not halt execution.
- * Instead, it will return the first successful result from a provider.
+ * A [MultiProvider.Strategy] similar to the [FirstMatchStrategy], except that errors from evaluated
+ * providers do not halt execution.
  *
- * If no provider successfully responds, it will throw an error result.
+ * If no provider successfully responds, it throws an error result.
  */
-class FirstSuccessfulStrategy : Strategy {
-    /**
-     * Evaluates providers in sequence until finding one that returns a successful result.
-     *
-     * @param providers List of providers to evaluate in order
-     * @param key The feature flag key to evaluate
-     * @param defaultValue Value to use in provider evaluations
-     * @param evaluationContext Optional context for evaluation
-     * @param flagEval The specific evaluation method to call on each provider
-     * @return ProviderEvaluation with the first successful result
-     * @throws OpenFeatureError.GeneralError if no provider returns a successful evaluation
-     */
+class FirstSuccessfulStrategy : MultiProvider.Strategy {
     override fun <T> evaluate(
         providers: List<FeatureProvider>,
         key: String,

kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/MultiProvider.kt

@@ -21,6 +21,17 @@ import kotlinx.coroutines.flow.launchIn
 import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.flow.update
 
+/**
+ * Type alias for a function that evaluates a feature flag using a FeatureProvider.
+ * This represents an extension function on FeatureProvider that takes:
+ * - key: The feature flag key to evaluate
+ * - defaultValue: The default value to return if evaluation fails
+ * - evaluationContext: Optional context for the evaluation
+ * Returns a ProviderEvaluation containing the result
+ */
+typealias FlagEval<T> =
+    FeatureProvider.(key: String, defaultValue: T, evaluationContext: EvaluationContext?) -> ProviderEvaluation<T>
+
 /**
  * MultiProvider is a FeatureProvider implementation that delegates flag evaluations
  * to multiple underlying providers using a configurable strategy.
@@ -51,6 +62,41 @@ class MultiProvider(
         val name: String // Maybe there's a better variable name for this?
     ) : FeatureProvider by implementation
 
+    /**
+     * Strategy interface defines how multiple feature providers should be evaluated
+     * to determine the final result for a feature flag evaluation.
+     * Different strategies can implement different logic for combining or selecting
+     * results from multiple providers.
+     */
+    interface Strategy {
+        /**
+         * Evaluates a feature flag across multiple providers using the strategy's logic.
+         * @param providers List of FeatureProvider instances to evaluate against
+         * @param key The feature flag key to evaluate
+         * @param defaultValue The default value to use if evaluation fails or no providers match
+         * @param evaluationContext Optional context containing additional data for evaluation
+         * @param flagEval Function reference to the specific evaluation method to call on each provider
+         * @return ProviderEvaluation<T> containing the final evaluation result
+         */
+        fun <T> evaluate(
+            providers: List<FeatureProvider>,
+            key: String,
+            defaultValue: T,
+            evaluationContext: EvaluationContext?,
+            flagEval: FlagEval<T>
+        ): ProviderEvaluation<T>
+    }
+
+    private val OpenFeatureStatus.precedence: Int
+        get() = when (this) {
+            is OpenFeatureStatus.Fatal -> 5
+            is OpenFeatureStatus.NotReady -> 4
+            is OpenFeatureStatus.Error -> 3
+            is OpenFeatureStatus.Reconciling -> 2 // Not specified in precedence; treat similar to Stale
+            is OpenFeatureStatus.Stale -> 2
+            is OpenFeatureStatus.Ready -> 1
+        }
+
     // TODO: Support hooks
     override val hooks: List<Hook<*>> = emptyList()
     private val childFeatureProviders: List<ChildFeatureProvider> by lazy {
@@ -61,11 +107,7 @@ class MultiProvider(
     override val metadata: ProviderMetadata = object : ProviderMetadata {
         override val name: String? = MULTIPROVIDER_NAME
         override val originalMetadata: Map<String, ProviderMetadata> by lazy {
-            constructOriginalMetadata()
-        }
-
-        private fun constructOriginalMetadata(): Map<String, ProviderMetadata> {
-            return childFeatureProviders.associate { it.name to it.metadata }
+            childFeatureProviders.associate { it.name to it.metadata }
         }
 
         override fun toString(): String {
@@ -114,7 +156,7 @@ class MultiProvider(
     /**
      * @return Number of unique providers
      */
-    fun getProviderCount(): Int = childFeatureProviders.size
+    internal fun getProviderCount(): Int = childFeatureProviders.size
 
     // TODO Add distinctUntilChanged operator once EventDetails have been added
     override fun observe(): Flow<OpenFeatureProviderEvents> = eventFlow.asSharedFlow()
@@ -145,12 +187,13 @@ class MultiProvider(
     }
 
     private suspend fun handleProviderEvent(provider: ChildFeatureProvider, event: OpenFeatureProviderEvents) {
-        if (event is OpenFeatureProviderEvents.ProviderConfigurationChanged) {
-            eventFlow.emit(event)
-            return
-        }
-
         val newChildStatus = when (event) {
+            // ProviderConfigurationChanged events should always re-emit
+            is OpenFeatureProviderEvents.ProviderConfigurationChanged -> {
+                eventFlow.emit(event)
+                return
+            }
+
             is OpenFeatureProviderEvents.ProviderReady -> OpenFeatureStatus.Ready
             is OpenFeatureProviderEvents.ProviderNotReady -> OpenFeatureStatus.NotReady
             is OpenFeatureProviderEvents.ProviderStale -> OpenFeatureStatus.Stale
@@ -160,8 +203,6 @@ class MultiProvider(
                 } else {
                     OpenFeatureStatus.Error(event.error)
                 }
-
-            else -> error("Unexpected event $event")
         }
 
         val previousStatus = _statusFlow.value
@@ -176,21 +217,10 @@ class MultiProvider(
     }
 
     private fun calculateAggregateStatus(): OpenFeatureStatus {
-        val highestPrecedenceStatus = childProviderStatuses.values.maxBy(::precedence)
+        val highestPrecedenceStatus = childProviderStatuses.values.maxBy { it.precedence }
         return highestPrecedenceStatus
     }
 
-    private fun precedence(status: OpenFeatureStatus): Int {
-        return when (status) {
-            is OpenFeatureStatus.Fatal -> 5
-            is OpenFeatureStatus.NotReady -> 4
-            is OpenFeatureStatus.Error -> 3
-            is OpenFeatureStatus.Reconciling -> 2 // Not specified in precedence; treat similar to Stale
-            is OpenFeatureStatus.Stale -> 2
-            is OpenFeatureStatus.Ready -> 1
-        }
-    }
-
     /**
      * Shuts down all underlying providers.
      * This allows providers to clean up resources and complete any pending operations.

kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/Strategy.kt

@@ -12,7 +12,7 @@ import kotlin.test.assertFailsWith
 class FirstMatchStrategyTests {
 
     @Test
-    fun returns_first_success_without_calling_next_providers() {
+    fun returnsFirstSuccessWithoutCallingNextProviders() {
         val strategy = FirstMatchStrategy()
         val first = RecordingBooleanProvider(
             name = "first",
@@ -38,7 +38,7 @@ class FirstMatchStrategyTests {
     }
 
     @Test
-    fun skips_flag_not_found_and_returns_next_match() {
+    fun skipsFlagNotFoundAndReturnsNextMatch() {
         val strategy = FirstMatchStrategy()
         val notFoundProvider = RecordingBooleanProvider(
             name = "not-found",
@@ -70,7 +70,7 @@ class FirstMatchStrategyTests {
     }
 
     @Test
-    fun treats_FlagNotFound_exception_as_not_found_and_continues() {
+    fun treatsFlagNotFoundExceptionAsNotFoundAndContinues() {
         val strategy = FirstMatchStrategy()
         val throwsNotFound = RecordingBooleanProvider(
             name = "throws-not-found",
@@ -95,7 +95,7 @@ class FirstMatchStrategyTests {
     }
 
     @Test
-    fun returns_error_result_other_than_not_found_and_short_circuits() {
+    fun returnsErrorResultOtherThanNotFoundAndShortCircuits() {
         val strategy = FirstMatchStrategy()
         val errorProvider = RecordingBooleanProvider(
             name = "error",
@@ -121,7 +121,7 @@ class FirstMatchStrategyTests {
     }
 
     @Test
-    fun bubbles_up_non_not_found_exceptions() {
+    fun bubblesUpNonNotFoundExceptions() {
         val strategy = FirstMatchStrategy()
         val throwsGeneral = RecordingBooleanProvider(
             name = "throws-general",
@@ -141,7 +141,7 @@ class FirstMatchStrategyTests {
     }
 
     @Test
-    fun returns_default_with_not_found_when_no_provider_matches() {
+    fun returnsDefaultWithNotFoundWhenNoProviderMatches() {
         val strategy = FirstMatchStrategy()
         val p1 = RecordingBooleanProvider(
             name = "p1",

kotlin-sdk/src/commonTest/kotlin/dev/openfeature/kotlin/sdk/multiprovider/FirstMatchStrategyTests.kt

@@ -11,7 +11,7 @@ import kotlin.test.assertFailsWith
 class FirstSuccessfulStrategyTests {
 
     @Test
-    fun returns_first_success_ignoring_prior_errors() {
+    fun returnsFirstSuccessIgnoringPriorErrors() {
         val strategy = FirstSuccessfulStrategy()
         val error1 = RecordingBooleanProvider("e1") {
             throw OpenFeatureError.GeneralError("boom1")
@@ -43,7 +43,7 @@ class FirstSuccessfulStrategyTests {
     }
 
     @Test
-    fun skips_flag_not_found_error_and_result_until_success() {
+    fun skipsFlagNotFoundErrorAndResultUntilSuccess() {
         val strategy = FirstSuccessfulStrategy()
         val notFoundThrow = RecordingBooleanProvider("nf-throw") {
             throw OpenFeatureError.FlagNotFoundError("flag")
@@ -70,7 +70,7 @@ class FirstSuccessfulStrategyTests {
     }
 
     @Test
-    fun throws_when_no_provider_returns_success() {
+    fun throwsWhenNoProviderReturnsSuccess() {
         val strategy = FirstSuccessfulStrategy()
         val error1 = RecordingBooleanProvider("e1") {
             throw OpenFeatureError.GeneralError("boom1")