fixup: feedback from guido

Signed-off-by: Todd Baert <[email protected]>

src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java

@@ -103,7 +103,6 @@ public Client getClient(String domain) {
      */
     public Client getClient(String domain, String version) {
         return new OpenFeatureClient(
-                () -> providerRepository.getFeatureProviderStateManager(domain),
                 this,
                 domain,
                 version
@@ -285,28 +284,6 @@ public FeatureProvider getProvider(String domain) {
         return providerRepository.getProvider(domain);
     }
 
-    /**
-     * Return the state of the default provider.
-     */
-    public ProviderState getProviderState() {
-        return providerRepository.getProviderState();
-    }
-
-    /**
-     * Get the state of the provider for a domain. If no provider with the domain is found, returns the state of the
-     * default provider.
-     *
-     * @param domain The domain to look for.
-     * @return A named {@link FeatureProvider}
-     */
-    public ProviderState getProviderState(String domain) {
-        return providerRepository.getProviderState(domain);
-    }
-
-    public ProviderState getProviderState(FeatureProvider provider) {
-        return providerRepository.getProviderState(provider);
-    }
-
     /**
      * Adds hooks for globally, used for all evaluations.
      * Hooks are run in the order they're added in the before stage. They are run in reverse order for all other stages.
@@ -424,6 +401,11 @@ void addHandler(String domain, ProviderEvent event, Consumer<EventDetails> handl
         }
     }
 
+    FeatureProviderStateManager getFeatureProviderStateManager(String domain) {
+        return providerRepository.getFeatureProviderStateManager(domain);
+    }
+
+
     /**
      * Runs the handlers associated with a particular provider.
      *

src/main/java/dev/openfeature/sdk/OpenFeatureClient.java

@@ -24,7 +24,6 @@
 @Deprecated() // TODO: eventually we will make this non-public. See issue #872
 public class OpenFeatureClient implements Client {
 
-    private final ProviderAccessor providerAccessor;
     private final OpenFeatureAPI openfeatureApi;
     @Getter
     private final String domain;
@@ -48,12 +47,10 @@ public class OpenFeatureClient implements Client {
      */
     @Deprecated() // TODO: eventually we will make this non-public. See issue #872
     public OpenFeatureClient(
-            ProviderAccessor providerAccessor,
             OpenFeatureAPI openFeatureAPI,
             String domain,
             String version
     ) {
-        this.providerAccessor = providerAccessor;
         this.openfeatureApi = openFeatureAPI;
         this.domain = domain;
         this.version = version;
@@ -63,7 +60,7 @@ public OpenFeatureClient(
 
     @Override
     public ProviderState getProviderState() {
-        return providerAccessor.getProviderStateManager().getState();
+        return openfeatureApi.getFeatureProviderStateManager(domain).getState();
     }
 
     /**
@@ -121,8 +118,8 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
         FeatureProvider provider;
 
         try {
-            // providerAccessor.getProviderStateManager() must be called once to maintain a consistent reference
-            FeatureProviderStateManager stateManager = providerAccessor.getProviderStateManager();
+            FeatureProviderStateManager stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain);
+            // provider must be accessed once to maintain a consistent reference
             provider = stateManager.getProvider();
             ProviderState state = stateManager.getState();
             if (ProviderState.NOT_READY.equals(state)) {

src/test/java/dev/openfeature/sdk/EventsTest.java

@@ -535,12 +535,12 @@ void matchingStaleEventsMustRunImmediately() {
 
         // provider which is already stale
         TestEventsProvider provider = TestEventsProvider.newInitializedTestEventsProvider();
+        Client client = api.getClient(name);
         api.setProviderAndWait(name, provider);
         provider.emitProviderStale(ProviderEventDetails.builder().build());
-        assertThat(api.getProviderState(name)).isEqualTo(ProviderState.STALE);
+        assertThat(client.getProviderState()).isEqualTo(ProviderState.STALE);
 
         // should run even thought handler was added after stale
-        Client client = api.getClient(name);
         client.onProviderStale(handler);
         verify(handler, timeout(TIMEOUT)).accept(any());
     }
@@ -555,13 +555,13 @@ void matchingErrorEventsMustRunImmediately() {
 
         // provider which is already in error
         TestEventsProvider provider = new TestEventsProvider();
+        Client client = api.getClient(name);
         api.setProviderAndWait(name, provider);
         provider.emitProviderError(ProviderEventDetails.builder().build());
-        assertThat(api.getProviderState(name)).isEqualTo(ProviderState.ERROR);
+        assertThat(client.getProviderState()).isEqualTo(ProviderState.ERROR);
 
 
         // should run even thought handler was added after error
-        Client client = api.getClient(name);
         client.onProviderError(handler);
         verify(handler, timeout(TIMEOUT)).accept(any());
     }

src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java

@@ -78,12 +78,14 @@ void getApiInstance() {
     @Test void providerAndWait() {
         FeatureProvider provider = new TestEventsProvider(500);
         OpenFeatureAPI.getInstance().setProviderAndWait(provider);
-        assertThat(api.getProviderState()).isEqualTo(ProviderState.READY);
+        Client client = api.getClient();
+        assertThat(client.getProviderState()).isEqualTo(ProviderState.READY);
 
         provider = new TestEventsProvider(500);
         String providerName = "providerAndWait";
         OpenFeatureAPI.getInstance().setProviderAndWait(providerName, provider);
-        assertThat(api.getProviderState(providerName)).isEqualTo(ProviderState.READY);
+        Client client2 = api.getClient(providerName);
+        assertThat(client2.getProviderState()).isEqualTo(ProviderState.READY);
     }
 
     @SneakyThrows
@@ -102,7 +104,6 @@ void getApiInstance() {
         FeatureProvider provider = new TestEventsProvider(100);
         String providerName = "shouldReturnNotReadyIfNotInitialized";
         OpenFeatureAPI.getInstance().setProvider(providerName, provider);
-        assertThat(api.getProviderState(providerName)).isEqualTo(ProviderState.NOT_READY);
         Client client = OpenFeatureAPI.getInstance().getClient(providerName);
         FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("return_error_when_not_initialized", false);
         assertEquals(ErrorCode.PROVIDER_NOT_READY, details.getErrorCode());

src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java

@@ -82,7 +82,7 @@ void settingTransactionalContextPropagatorToNullErrors() {
 
     @Test
     void setEvaluationContextShouldAllowChaining() {
-        OpenFeatureClient client = new OpenFeatureClient(() -> null, api, "name", "version");
+        OpenFeatureClient client = new OpenFeatureClient(api, "name", "version");
         EvaluationContext ctx = new ImmutableContext("targeting key", new HashMap<>());
         OpenFeatureClient result = client.setEvaluationContext(ctx);
         assertEquals(client, result);

src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java

@@ -1,7 +1,16 @@
 package dev.openfeature.sdk;
 
-import dev.openfeature.sdk.fixtures.HookFixtures;
-import lombok.SneakyThrows;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+
+import java.util.HashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
@@ -10,14 +19,9 @@
 import org.simplify4u.slf4jmock.LoggerMock;
 import org.slf4j.Logger;
 
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.concurrent.atomic.AtomicBoolean;
-
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.mockito.ArgumentMatchers.*;
-import static org.mockito.Mockito.*;
+import dev.openfeature.sdk.exceptions.FatalError;
+import dev.openfeature.sdk.fixtures.HookFixtures;
+import dev.openfeature.sdk.testutils.TestEventsProvider;
 
 class OpenFeatureClientTest implements HookFixtures {
 
@@ -37,14 +41,10 @@ void reset_logs() {
     @Test
     @DisplayName("should not throw exception if hook has different type argument than hookContext")
     void shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext() {
-        DoSomethingProvider provider = new DoSomethingProvider();
-        OpenFeatureAPI api = mock(OpenFeatureAPI.class);
-        when(api.getProvider(any())).thenReturn(provider);
-        when(api.getHooks()).thenReturn(Arrays.asList(mockBooleanHook(), mockStringHook()));
-
-        MockProviderRepository mockProviderRepository = new MockProviderRepository(provider, true);
-        OpenFeatureClient client = new OpenFeatureClient(mockProviderRepository, api, "name", "version");
-
+        OpenFeatureAPI api = OpenFeatureAPI.getInstance();
+        api.setProvider("shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext", new DoSomethingProvider());
+        Client client = api.getClient("shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext");
+        client.addHooks(mockBooleanHook(), mockStringHook());
         FlagEvaluationDetails<Boolean> actual = client.getBooleanDetails("feature key", Boolean.FALSE);
 
         assertThat(actual.getValue()).isTrue();
@@ -56,36 +56,11 @@ void shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext() {
         Mockito.verify(logger, never()).error(anyString(), any(), any());
     }
 
-    @Test
-    void mergeContextTest() {
-        String flag = "feature key";
-        boolean defaultValue = false;
-        String targetingKey = "targeting key";
-        EvaluationContext ctx = new ImmutableContext(targetingKey, new HashMap<>());
-        OpenFeatureAPI api = mock(OpenFeatureAPI.class);
-        FeatureProvider mockProvider = mock(FeatureProvider.class);
-        // this makes it so that true is returned only if the targeting key set at the client level is honored
-        when(mockProvider.getBooleanEvaluation(
-                eq(flag), eq(defaultValue), argThat(
-                        context -> context.getTargetingKey().equals(targetingKey)))).thenReturn(ProviderEvaluation.<Boolean>builder()
-                .value(true).build());
-        when(api.getProvider()).thenReturn(mockProvider);
-        when(api.getProvider(any())).thenReturn(mockProvider);
-
-        MockProviderRepository mockProviderRepository = new MockProviderRepository(mockProvider, true);
-        OpenFeatureClient client = new OpenFeatureClient(mockProviderRepository, api, "name", "version");
-        client.setEvaluationContext(ctx);
-
-        FlagEvaluationDetails<Boolean> result = client.getBooleanDetails(flag, defaultValue);
-
-        assertThat(result.getValue()).isTrue();
-    }
-
     @Test
     @DisplayName("addHooks should allow chaining by returning the same client instance")
     void addHooksShouldAllowChaining() {
         OpenFeatureAPI api = mock(OpenFeatureAPI.class);
-        OpenFeatureClient client = new OpenFeatureClient(() -> null, api, "name", "version");
+        OpenFeatureClient client = new OpenFeatureClient(api, "name", "version");
         Hook<?> hook1 = Mockito.mock(Hook.class);
         Hook<?> hook2 = Mockito.mock(Hook.class);
 
@@ -97,7 +72,7 @@ void addHooksShouldAllowChaining() {
     @DisplayName("setEvaluationContext should allow chaining by returning the same client instance")
     void setEvaluationContextShouldAllowChaining() {
         OpenFeatureAPI api = mock(OpenFeatureAPI.class);
-        OpenFeatureClient client = new OpenFeatureClient(() -> null, api, "name", "version");
+        OpenFeatureClient client = new OpenFeatureClient(api, "name", "version");
         EvaluationContext ctx = new ImmutableContext("targeting key", new HashMap<>());
 
         OpenFeatureClient result = client.setEvaluationContext(ctx);
@@ -107,49 +82,27 @@ void setEvaluationContextShouldAllowChaining() {
     @Test
     @DisplayName("Should not call evaluation methods when the provider has state FATAL")
     void shouldNotCallEvaluationMethodsWhenProviderIsInFatalErrorState() {
-        MockProvider mockProvider = new MockProvider(ProviderState.FATAL);
-        OpenFeatureAPI api = mock(OpenFeatureAPI.class);
-        MockProviderRepository mockProviderRepository = new MockProviderRepository(mockProvider, true);
-        OpenFeatureClient client = new OpenFeatureClient(mockProviderRepository, api, "name", "version");
-        mockProviderRepository.featureProviderStateManager.onEmit(
-                ProviderEvent.PROVIDER_ERROR,
-                ProviderEventDetails.builder().errorCode(ErrorCode.PROVIDER_FATAL).build()
-        );
-        FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("key", true);
+        FeatureProvider provider = new TestEventsProvider(100, true, "fake fatal", true);
+        OpenFeatureAPI api = OpenFeatureAPI.getInstance();
+        Client client = api.getClient("shouldNotCallEvaluationMethodsWhenProviderIsInFatalErrorState");
 
-        assertThat(mockProvider.isEvaluationCalled()).isFalse();
+        assertThrows(FatalError.class, () -> api.setProviderAndWait("shouldNotCallEvaluationMethodsWhenProviderIsInFatalErrorState", provider));
+        FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("key", true);
         assertThat(details.getErrorCode()).isEqualTo(ErrorCode.PROVIDER_FATAL);
     }
 
     @Test
     @DisplayName("Should not call evaluation methods when the provider has state NOT_READY")
     void shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState() {
-        MockProvider mockProvider = new MockProvider(ProviderState.NOT_READY);
-        OpenFeatureAPI api = mock(OpenFeatureAPI.class);
-        OpenFeatureClient client = new OpenFeatureClient(new MockProviderRepository(mockProvider, false), api, "name", "version");
+        FeatureProvider provider = new TestEventsProvider(5000);
+        OpenFeatureAPI api = OpenFeatureAPI.getInstance();
+        api.setProvider("shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState", provider);
+        Client client = api.getClient("shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState");
         FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("key", true);
 
-        assertThat(mockProvider.isEvaluationCalled()).isFalse();
         assertThat(details.getErrorCode()).isEqualTo(ErrorCode.PROVIDER_NOT_READY);
     }
 
-    private static class MockProviderRepository implements ProviderAccessor {
-        private final FeatureProviderStateManager featureProviderStateManager;
-
-        @SneakyThrows
-        public MockProviderRepository(FeatureProvider featureProvider, boolean init) {
-            this.featureProviderStateManager = new FeatureProviderStateManager(featureProvider);
-            if (init) {
-                this.featureProviderStateManager.initialize(null);
-            }
-        }
-
-        @Override
-        public FeatureProviderStateManager getProviderStateManager() {
-            return featureProviderStateManager;
-        }
-    }
-
     private static class MockProvider implements FeatureProvider {
         private final AtomicBoolean evaluationCalled = new AtomicBoolean();
         private final ProviderState providerState;

src/test/java/dev/openfeature/sdk/testutils/TestEventsProvider.java

@@ -1,6 +1,7 @@
 package dev.openfeature.sdk.testutils;
 
 import dev.openfeature.sdk.*;
+import dev.openfeature.sdk.exceptions.FatalError;
 import dev.openfeature.sdk.exceptions.GeneralError;
 import lombok.SneakyThrows;
 
@@ -13,6 +14,7 @@ public class TestEventsProvider extends EventProvider {
     private int initTimeoutMs = 0;
     private String name = "test";
     private Metadata metadata = () -> name;
+    private boolean isFatalInitError = false;
 
     public TestEventsProvider() {
     }
@@ -27,6 +29,13 @@ public TestEventsProvider(int initTimeoutMs, boolean initError, String initError
         this.initErrorMessage = initErrorMessage;
     }
 
+    public TestEventsProvider(int initTimeoutMs, boolean initError, String initErrorMessage, boolean fatal) {
+        this.initTimeoutMs = initTimeoutMs;
+        this.initError = initError;
+        this.initErrorMessage = initErrorMessage;
+        this.isFatalInitError = fatal;
+    }
+
     @SneakyThrows
     public static TestEventsProvider newInitializedTestEventsProvider() {
         TestEventsProvider provider = new TestEventsProvider();
@@ -52,6 +61,9 @@ public void initialize(EvaluationContext evaluationContext) throws Exception {
         // wait half the TIMEOUT, otherwise some init/errors can be fired before we add handlers
         Thread.sleep(initTimeoutMs);
         if (this.initError) {
+            if (this.isFatalInitError) {
+                throw new FatalError(initErrorMessage);
+            }
             throw new GeneralError(initErrorMessage);
         }
     }