From 45e9ba2e0b33ee84b0449003daff3f03cb820a45 Mon Sep 17 00:00:00 2001 From: Todd Baert Date: Mon, 23 Sep 2024 11:54:58 -0400 Subject: [PATCH] fixup: feedback from guido Signed-off-by: Todd Baert --- .../dev/openfeature/sdk/OpenFeatureAPI.java | 28 +---- .../openfeature/sdk/OpenFeatureClient.java | 9 +- .../dev/openfeature/sdk/ProviderAccessor.java | 9 -- .../java/dev/openfeature/sdk/EventsTest.java | 8 +- .../sdk/FlagEvaluationSpecTest.java | 7 +- .../openfeature/sdk/OpenFeatureAPITest.java | 2 +- .../sdk/OpenFeatureClientTest.java | 105 +++++------------- .../sdk/testutils/TestEventsProvider.java | 12 ++ 8 files changed, 58 insertions(+), 122 deletions(-) delete mode 100644 src/main/java/dev/openfeature/sdk/ProviderAccessor.java diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java index a06029d60..d087b5b2c 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java +++ b/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 handl } } + FeatureProviderStateManager getFeatureProviderStateManager(String domain) { + return providerRepository.getFeatureProviderStateManager(domain); + } + + /** * Runs the handlers associated with a particular provider. * diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 06e083464..2162f4130 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/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 FlagEvaluationDetails 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)) { diff --git a/src/main/java/dev/openfeature/sdk/ProviderAccessor.java b/src/main/java/dev/openfeature/sdk/ProviderAccessor.java deleted file mode 100644 index 0f1a4836d..000000000 --- a/src/main/java/dev/openfeature/sdk/ProviderAccessor.java +++ /dev/null @@ -1,9 +0,0 @@ -package dev.openfeature.sdk; - -/** - * Provides access to the future provider for the domain of the client. - */ -@FunctionalInterface -interface ProviderAccessor { - FeatureProviderStateManager getProviderStateManager(); -} diff --git a/src/test/java/dev/openfeature/sdk/EventsTest.java b/src/test/java/dev/openfeature/sdk/EventsTest.java index bab48ee58..41bcf86c4 100644 --- a/src/test/java/dev/openfeature/sdk/EventsTest.java +++ b/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()); } diff --git a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java b/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java index 4dd28f719..a2316a59c 100644 --- a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java +++ b/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 details = client.getBooleanDetails("return_error_when_not_initialized", false); assertEquals(ErrorCode.PROVIDER_NOT_READY, details.getErrorCode()); diff --git a/src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java b/src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java index 83b3ba195..23c758e9f 100644 --- a/src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java +++ b/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); diff --git a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java index d7bf70870..82199accf 100644 --- a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java +++ b/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 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.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 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 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 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 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; diff --git a/src/test/java/dev/openfeature/sdk/testutils/TestEventsProvider.java b/src/test/java/dev/openfeature/sdk/testutils/TestEventsProvider.java index 41e7bff4c..1944fce22 100644 --- a/src/test/java/dev/openfeature/sdk/testutils/TestEventsProvider.java +++ b/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); } }