From 506e89fd348f107df0065c5e0b218abdded0efec Mon Sep 17 00:00:00 2001 From: Liran M <77168114+liran2000@users.noreply.github.com> Date: Mon, 28 Aug 2023 22:43:59 +0300 Subject: [PATCH] feat: add method to set provider and block during init (#563) * feat: spec 1.1.8 - setProviderAndWait The API SHOULD provide functions to set a provider and wait for the initialize function to return or throw Signed-off-by: liran2000 * remove method overloading from package private class Signed-off-by: liran2000 * add test case for spec 2.4.5 Signed-off-by: liran2000 * minor updates Signed-off-by: liran2000 --------- Signed-off-by: liran2000 Co-authored-by: Todd Baert Co-authored-by: Michael Beemer --- .../dev/openfeature/sdk/OpenFeatureAPI.java | 55 +++++++++--- .../openfeature/sdk/ProviderRepository.java | 84 +++++++++++-------- .../sdk/FlagEvaluationSpecTest.java | 33 ++++++++ .../sdk/ProviderRepositoryTest.java | 20 ++--- .../openfeature/sdk/e2e/StepDefinitions.java | 6 +- .../memory/InMemoryProviderTest.java | 18 ++-- .../sdk/testutils/TestEventsProvider.java | 12 +-- 7 files changed, 158 insertions(+), 70 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java index 94047b8cf..47c093886 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java @@ -100,11 +100,12 @@ public EvaluationContext getEvaluationContext() { public void setProvider(FeatureProvider provider) { try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { providerRepository.setProvider( - provider, - (p) -> attachEventProvider(p), - (p) -> emitReady(p), - (p) -> detachEventProvider(p), - (p, message) -> emitError(p, message)); + provider, + this::attachEventProvider, + this::emitReady, + this::detachEventProvider, + this::emitError, + false); } } @@ -117,11 +118,45 @@ public void setProvider(FeatureProvider provider) { public void setProvider(String clientName, FeatureProvider provider) { try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { providerRepository.setProvider(clientName, - provider, - this::attachEventProvider, - this::emitReady, - this::detachEventProvider, - this::emitError); + provider, + this::attachEventProvider, + this::emitReady, + this::detachEventProvider, + this::emitError, + false); + } + } + + /** + * Set the default provider and wait for initialization to finish. + */ + public void setProviderAndWait(FeatureProvider provider) { + try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + providerRepository.setProvider( + provider, + this::attachEventProvider, + this::emitReady, + this::detachEventProvider, + this::emitError, + true); + } + } + + /** + * Add a provider for a named client and wait for initialization to finish. + * + * @param clientName The name of the client. + * @param provider The provider to set. + */ + public void setProviderAndWait(String clientName, FeatureProvider provider) { + try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) { + providerRepository.setProvider(clientName, + provider, + this::attachEventProvider, + this::emitReady, + this::detachEventProvider, + this::emitError, + true); } } diff --git a/src/main/java/dev/openfeature/sdk/ProviderRepository.java b/src/main/java/dev/openfeature/sdk/ProviderRepository.java index 0ff3b70be..b7d570498 100644 --- a/src/main/java/dev/openfeature/sdk/ProviderRepository.java +++ b/src/main/java/dev/openfeature/sdk/ProviderRepository.java @@ -43,8 +43,8 @@ public FeatureProvider getProvider(String name) { public List getClientNamesForProvider(FeatureProvider provider) { return providers.entrySet().stream() - .filter(entry -> entry.getValue().equals(provider)) - .map(entry -> entry.getKey()).collect(Collectors.toList()); + .filter(entry -> entry.getValue().equals(provider)) + .map(entry -> entry.getKey()).collect(Collectors.toList()); } public Set getAllBoundClientNames() { @@ -62,58 +62,76 @@ public void setProvider(FeatureProvider provider, Consumer afterSet, Consumer afterInit, Consumer afterShutdown, - BiConsumer afterError) { + BiConsumer afterError, + boolean waitForInit) { if (provider == null) { throw new IllegalArgumentException("Provider cannot be null"); } - initializeProvider(null, provider, afterSet, afterInit, afterShutdown, afterError); + prepareAndInitializeProvider(null, provider, afterSet, afterInit, afterShutdown, afterError, waitForInit); } /** * Add a provider for a named client. * - * @param clientName The name of the client. - * @param provider The provider to set. + * @param clientName The name of the client. + * @param provider The provider to set. + * @param waitForInit When true, wait for initialization to finish, then returns. + * Otherwise, initialization happens in the background. */ public void setProvider(String clientName, - FeatureProvider provider, - Consumer afterSet, - Consumer afterInit, - Consumer afterShutdown, - BiConsumer afterError) { + FeatureProvider provider, + Consumer afterSet, + Consumer afterInit, + Consumer afterShutdown, + BiConsumer afterError, + boolean waitForInit) { if (provider == null) { throw new IllegalArgumentException("Provider cannot be null"); } if (clientName == null) { throw new IllegalArgumentException("clientName cannot be null"); } - initializeProvider(clientName, provider, afterSet, afterInit, afterShutdown, afterError); + prepareAndInitializeProvider(clientName, provider, afterSet, afterInit, afterShutdown, afterError, waitForInit); } - private void initializeProvider(@Nullable String clientName, - FeatureProvider newProvider, - Consumer afterSet, - Consumer afterInit, - Consumer afterShutdown, - BiConsumer afterError) { + private void prepareAndInitializeProvider(@Nullable String clientName, + FeatureProvider newProvider, + Consumer afterSet, + Consumer afterInit, + Consumer afterShutdown, + BiConsumer afterError, + boolean waitForInit) { + // provider is set immediately, on this thread FeatureProvider oldProvider = clientName != null - ? this.providers.put(clientName, newProvider) - : this.defaultProvider.getAndSet(newProvider); + ? this.providers.put(clientName, newProvider) + : this.defaultProvider.getAndSet(newProvider); afterSet.accept(newProvider); - taskExecutor.submit(() -> { - // initialization happens in a different thread - try { - if (ProviderState.NOT_READY.equals(newProvider.getState())) { - newProvider.initialize(OpenFeatureAPI.getInstance().getEvaluationContext()); - afterInit.accept(newProvider); - } - shutDownOld(oldProvider, afterShutdown); - } catch (Exception e) { - log.error("Exception when initializing feature provider {}", newProvider.getClass().getName(), e); - afterError.accept(newProvider, e.getMessage()); + if (waitForInit) { + initializeProvider(newProvider, afterInit, afterShutdown, afterError, oldProvider); + } else { + taskExecutor.submit(() -> { + // initialization happens in a different thread if we're not waiting it + initializeProvider(newProvider, afterInit, afterShutdown, afterError, oldProvider); + }); + } + } + + private void initializeProvider(FeatureProvider newProvider, + Consumer afterInit, + Consumer afterShutdown, + BiConsumer afterError, + FeatureProvider oldProvider) { + try { + if (ProviderState.NOT_READY.equals(newProvider.getState())) { + newProvider.initialize(OpenFeatureAPI.getInstance().getEvaluationContext()); + afterInit.accept(newProvider); } - }); + shutDownOld(oldProvider, afterShutdown); + } catch (Exception e) { + log.error("Exception when initializing feature provider {}", newProvider.getClass().getName(), e); + afterError.accept(newProvider, e.getMessage()); + } } private void shutDownOld(FeatureProvider oldProvider,Consumer afterShutdown) { @@ -157,7 +175,7 @@ public void shutdown() { }, (FeatureProvider fp, String message) -> { - }); + }, false); this.providers.clear(); taskExecutor.shutdown(); } diff --git a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java b/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java index eb41fd950..35eb0769c 100644 --- a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java +++ b/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java @@ -17,6 +17,10 @@ import java.util.List; import java.util.Map; +import dev.openfeature.sdk.providers.memory.InMemoryProvider; +import dev.openfeature.sdk.testutils.TestEventsProvider; +import lombok.SneakyThrows; +import org.awaitility.Awaitility; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -69,6 +73,34 @@ void getApiInstance() { assertThat(api.getProvider()).isEqualTo(mockProvider); } + @SneakyThrows + @Specification(number="1.1.8", text="The API SHOULD provide functions to set a provider and wait for the initialize function to return or throw.") + @Test void providerAndWait() { + FeatureProvider provider = new TestEventsProvider(500); + OpenFeatureAPI.getInstance().setProviderAndWait(provider); + assertThat(api.getProvider().getState()).isEqualTo(ProviderState.READY); + + provider = new TestEventsProvider(500); + String providerName = "providerAndWait"; + OpenFeatureAPI.getInstance().setProviderAndWait(providerName, provider); + assertThat(api.getProvider(providerName).getState()).isEqualTo(ProviderState.READY); + } + + @Specification(number="2.4.5", text="The provider SHOULD indicate an error if flag resolution is attempted before the provider is ready.") + @Test void shouldReturnNotReadyIfNotInitialized() { + FeatureProvider provider = new InMemoryProvider(new HashMap<>()) { + @Override + public void initialize(EvaluationContext evaluationContext) throws Exception { + Awaitility.await().wait(3000); + } + }; + String providerName = "shouldReturnNotReadyIfNotInitialized"; + OpenFeatureAPI.getInstance().setProvider(providerName, provider); + assertThat(api.getProvider(providerName).getState()).isEqualTo(ProviderState.NOT_READY); + Client client = OpenFeatureAPI.getInstance().getClient(providerName); + assertEquals(ErrorCode.PROVIDER_NOT_READY, client.getBooleanDetails("return_error_when_not_initialized", false).getErrorCode()); + } + @Specification(number="1.1.5", text="The API MUST provide a function for retrieving the metadata field of the configured provider.") @Test void provider_metadata() { FeatureProviderTestUtils.setFeatureProvider(new DoSomethingProvider()); @@ -291,4 +323,5 @@ void getApiInstance() { @Specification(number="1.4.11", text="The client SHOULD provide asynchronous or non-blocking mechanisms for flag evaluation.") @Test void one_thread_per_request_model() {} + } diff --git a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java index 5b6dac1b5..0d4ae5d6a 100644 --- a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java +++ b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java @@ -55,7 +55,7 @@ class DefaultProvider { @DisplayName("should reject null as default provider") void shouldRejectNullAsDefaultProvider() { assertThatCode(() -> providerRepository.setProvider(null, mockAfterSet(), mockAfterInit(), - mockAfterShutdown(), mockAfterError())).isInstanceOf(IllegalArgumentException.class); + mockAfterShutdown(), mockAfterError(), false)).isInstanceOf(IllegalArgumentException.class); } @Test @@ -76,7 +76,7 @@ void shouldImmediatelyReturnWhenCallingTheProviderMutator() throws Exception { .atMost(Duration.ofSeconds(1)) .until(() -> { providerRepository.setProvider(featureProvider, mockAfterSet(), mockAfterInit(), - mockAfterShutdown(), mockAfterError()); + mockAfterShutdown(), mockAfterError(), false); verify(featureProvider, timeout(TIMEOUT)).initialize(any()); return true; }); @@ -101,7 +101,7 @@ class NamedProvider { @DisplayName("should reject null as named provider") void shouldRejectNullAsNamedProvider() { assertThatCode(() -> providerRepository.setProvider(CLIENT_NAME, null, mockAfterSet(), mockAfterInit(), - mockAfterShutdown(), mockAfterError())) + mockAfterShutdown(), mockAfterError(), false)) .isInstanceOf(IllegalArgumentException.class); } @@ -110,7 +110,7 @@ void shouldRejectNullAsNamedProvider() { void shouldRejectNullAsDefaultProvider() { NoOpProvider provider = new NoOpProvider(); assertThatCode(() -> providerRepository.setProvider(null, provider, mockAfterSet(), mockAfterInit(), - mockAfterShutdown(), mockAfterError())) + mockAfterShutdown(), mockAfterError(), false)) .isInstanceOf(IllegalArgumentException.class); } @@ -126,7 +126,7 @@ void shouldImmediatelyReturnWhenCallingTheNamedClientProviderMutator() throws Ex .atMost(Duration.ofSeconds(1)) .until(() -> { providerRepository.setProvider("named client", featureProvider, mockAfterSet(), - mockAfterInit(), mockAfterShutdown(), mockAfterError()); + mockAfterInit(), mockAfterShutdown(), mockAfterError(), false); verify(featureProvider, timeout(TIMEOUT)).initialize(any()); return true; }); @@ -161,7 +161,7 @@ void shouldImmediatelyReturnWhenCallingTheProviderMutator() throws Exception { .atMost(Duration.ofSeconds(1)) .until(() -> { providerRepository.setProvider(newProvider, mockAfterSet(), mockAfterInit(), - mockAfterShutdown(), mockAfterError()); + mockAfterShutdown(), mockAfterError(), false); verify(newProvider, timeout(TIMEOUT)).initialize(any()); return true; }); @@ -194,7 +194,7 @@ void shouldImmediatelyReturnWhenCallingTheProviderMutator() throws Exception { Future providerMutation = executorService .submit(() -> providerRepository.setProvider(CLIENT_NAME, newProvider, mockAfterSet(), - mockAfterInit(), mockAfterShutdown(), mockAfterError())); + mockAfterInit(), mockAfterShutdown(), mockAfterError(), false)); await() .alias("wait for provider mutator to return") @@ -311,7 +311,7 @@ void shouldShutdownAllFeatureProvidersOnShutdown() { private void setFeatureProvider(FeatureProvider provider) { providerRepository.setProvider(provider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), - mockAfterError()); + mockAfterError(), false); waitForSettingProviderHasBeenCompleted(ProviderRepository::getProvider, provider); } @@ -320,13 +320,13 @@ private void setFeatureProvider(FeatureProvider provider, Consumer afterInit, Consumer afterShutdown, BiConsumer afterError) { providerRepository.setProvider(provider, afterSet, afterInit, afterShutdown, - afterError); + afterError, false); waitForSettingProviderHasBeenCompleted(ProviderRepository::getProvider, provider); } private void setFeatureProvider(String namedProvider, FeatureProvider provider) { providerRepository.setProvider(namedProvider, provider, mockAfterSet(), mockAfterInit(), mockAfterShutdown(), - mockAfterError()); + mockAfterError(), false); waitForSettingProviderHasBeenCompleted(repository -> repository.getProvider(namedProvider), provider); } diff --git a/src/test/java/dev/openfeature/sdk/e2e/StepDefinitions.java b/src/test/java/dev/openfeature/sdk/e2e/StepDefinitions.java index 903f0bf8e..459fcefea 100644 --- a/src/test/java/dev/openfeature/sdk/e2e/StepDefinitions.java +++ b/src/test/java/dev/openfeature/sdk/e2e/StepDefinitions.java @@ -56,11 +56,7 @@ public class StepDefinitions { public static void setup() { Map> flags = buildFlags(); InMemoryProvider provider = new InMemoryProvider(flags); - OpenFeatureAPI.getInstance().setProvider(provider); - - // TODO: setProvider with wait for init, pending https://github.com/open-feature/ofep/pull/80 - Thread.sleep(500); - + OpenFeatureAPI.getInstance().setProviderAndWait(provider); client = OpenFeatureAPI.getInstance().getClient(); } diff --git a/src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java b/src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java index 4cfbb8124..ffdc31822 100644 --- a/src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java +++ b/src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java @@ -6,12 +6,13 @@ import dev.openfeature.sdk.OpenFeatureAPI; import dev.openfeature.sdk.Value; import dev.openfeature.sdk.exceptions.FlagNotFoundError; +import dev.openfeature.sdk.exceptions.ProviderNotReadyError; import dev.openfeature.sdk.exceptions.TypeMismatchError; import lombok.SneakyThrows; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; -import org.omg.CORBA.DynAnyPackage.TypeMismatch; +import java.util.HashMap; import java.util.Map; import static dev.openfeature.sdk.Structure.mapToStructure; @@ -36,11 +37,7 @@ static void beforeAll() { Map> flags = buildFlags(); provider = spy(new InMemoryProvider(flags)); OpenFeatureAPI.getInstance().onProviderConfigurationChanged(eventDetails -> {}); - OpenFeatureAPI.getInstance().setProvider(provider); - - // TODO: setProvider with wait for init, pending https://github.com/open-feature/ofep/pull/80 - Thread.sleep(500); - + OpenFeatureAPI.getInstance().setProviderAndWait(provider); client = OpenFeatureAPI.getInstance().getClient(); provider.updateFlags(flags); provider.updateFlag("addedFlag", Flag.builder() @@ -99,4 +96,13 @@ void typeMismatch() { provider.getBooleanEvaluation("string-flag", false, new ImmutableContext()); }); } + + @SneakyThrows + @Test + void shouldThrowIfNotInitialized() { + InMemoryProvider inMemoryProvider = new InMemoryProvider(new HashMap<>()); + + // ErrorCode.PROVIDER_NOT_READY should be returned when evaluated via the client + assertThrows(ProviderNotReadyError.class, ()-> inMemoryProvider.getBooleanEvaluation("fail_not_initialized", false, new ImmutableContext())); + } } \ No newline at end of file diff --git a/src/test/java/dev/openfeature/sdk/testutils/TestEventsProvider.java b/src/test/java/dev/openfeature/sdk/testutils/TestEventsProvider.java index 3fcb58886..25650bf61 100644 --- a/src/test/java/dev/openfeature/sdk/testutils/TestEventsProvider.java +++ b/src/test/java/dev/openfeature/sdk/testutils/TestEventsProvider.java @@ -15,19 +15,19 @@ public class TestEventsProvider extends EventProvider { private String initErrorMessage; private ProviderState state = ProviderState.NOT_READY; private boolean shutDown = false; - private int initTimeout = 0; + private int initTimeoutMs = 0; @Override public ProviderState getState() { return this.state; } - public TestEventsProvider(int initTimeout) { - this.initTimeout = initTimeout; + public TestEventsProvider(int initTimeoutMs) { + this.initTimeoutMs = initTimeoutMs; } - public TestEventsProvider(int initTimeout, boolean initError, String initErrorMessage) { - this.initTimeout = initTimeout; + public TestEventsProvider(int initTimeoutMs, boolean initError, String initErrorMessage) { + this.initTimeoutMs = initTimeoutMs; this.initError = initError; this.initErrorMessage = initErrorMessage; } @@ -53,7 +53,7 @@ public void shutdown() { public void initialize(EvaluationContext evaluationContext) throws Exception { if (ProviderState.NOT_READY.equals(state)) { // wait half the TIMEOUT, otherwise some init/errors can be fired before we add handlers - Thread.sleep(initTimeout); + Thread.sleep(initTimeoutMs); if (this.initError) { this.state = ProviderState.ERROR; throw new Exception(initErrorMessage);