From 3319e558700d743ed187561695b4d51f51664390 Mon Sep 17 00:00:00 2001 From: Kavindu Dodanduwa Date: Tue, 12 Dec 2023 12:43:14 -0800 Subject: [PATCH] fix: tolerate duplicate provider registrations (#725) * fix provider mulitple regiration issue Signed-off-by: Kavindu Dodanduwa * fix lint Signed-off-by: Kavindu Dodanduwa * fix tests Signed-off-by: Kavindu Dodanduwa * improve test and add check for unused imports Signed-off-by: Kavindu Dodanduwa --------- Signed-off-by: Kavindu Dodanduwa --- checkstyle.xml | 3 ++- .../dev/openfeature/sdk/EventProvider.java | 4 ++- .../openfeature/sdk/ProviderRepository.java | 26 +++++++++++++------ .../openfeature/sdk/OpenFeatureAPITest.java | 23 ++++++++++++++++ .../sdk/ProviderRepositoryTest.java | 1 + .../testutils/FeatureProviderTestUtils.java | 1 + 6 files changed, 48 insertions(+), 10 deletions(-) diff --git a/checkstyle.xml b/checkstyle.xml index f2bcb59a..2cf598c3 100644 --- a/checkstyle.xml +++ b/checkstyle.xml @@ -63,7 +63,8 @@ default="checkstyle-xpath-suppressions.xml" /> - + + diff --git a/src/main/java/dev/openfeature/sdk/EventProvider.java b/src/main/java/dev/openfeature/sdk/EventProvider.java index 928b96f4..4e8c022b 100644 --- a/src/main/java/dev/openfeature/sdk/EventProvider.java +++ b/src/main/java/dev/openfeature/sdk/EventProvider.java @@ -28,7 +28,9 @@ public abstract class EventProvider implements FeatureProvider { * "Attach" this EventProvider to an SDK, which allows events to propagate from this provider. * No-op if the same onEmit is already attached. * - * @param onEmit the function to run when a provider emits events. + * @param onEmit the function to run when a provider emits events. + * @throws IllegalStateException if attempted to bind a new emitter for already bound provider + * */ void attach(TriConsumer onEmit) { if (this.onEmit != null && this.onEmit != onEmit) { diff --git a/src/main/java/dev/openfeature/sdk/ProviderRepository.java b/src/main/java/dev/openfeature/sdk/ProviderRepository.java index 2ca3b21f..cea835e8 100644 --- a/src/main/java/dev/openfeature/sdk/ProviderRepository.java +++ b/src/main/java/dev/openfeature/sdk/ProviderRepository.java @@ -106,11 +106,16 @@ class ProviderRepository { BiConsumer afterError, boolean waitForInit) { + if (!isProviderRegistered(newProvider)) { + // only run afterSet if new provider is not already attached + afterSet.accept(newProvider); + } + // provider is set immediately, on this thread FeatureProvider oldProvider = clientName != null - ? this.providers.put(clientName, newProvider) - : this.defaultProvider.getAndSet(newProvider); - afterSet.accept(newProvider); + ? this.providers.put(clientName, newProvider) + : this.defaultProvider.getAndSet(newProvider); + if (waitForInit) { initializeProvider(newProvider, afterInit, afterShutdown, afterError, oldProvider); } else { @@ -138,16 +143,21 @@ class ProviderRepository { } } - private void shutDownOld(FeatureProvider oldProvider,Consumer afterShutdown) { - if (oldProvider != null && !isProviderRegistered(oldProvider)) { + private void shutDownOld(FeatureProvider oldProvider, Consumer afterShutdown) { + if (!isProviderRegistered(oldProvider)) { shutdownProvider(oldProvider); afterShutdown.accept(oldProvider); } } - private boolean isProviderRegistered(FeatureProvider oldProvider) { - return oldProvider != null && (this.providers.containsValue(oldProvider) - || this.defaultProvider.get().equals(oldProvider)); + /** + * Helper to check if provider is already known (registered). + * @param provider provider to check for registration + * @return boolean true if already registered, false otherwise + */ + private boolean isProviderRegistered(FeatureProvider provider) { + return provider != null + && (this.providers.containsValue(provider) || this.defaultProvider.get().equals(provider)); } private void shutdownProvider(FeatureProvider provider) { diff --git a/src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java b/src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java index 10b38e6c..3b0e8956 100644 --- a/src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java +++ b/src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java @@ -1,11 +1,15 @@ package dev.openfeature.sdk; +import dev.openfeature.sdk.providers.memory.InMemoryProvider; import dev.openfeature.sdk.testutils.FeatureProviderTestUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import java.util.Collections; + import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; +import static org.junit.jupiter.api.Assertions.assertEquals; class OpenFeatureAPITest { @@ -40,6 +44,25 @@ class OpenFeatureAPITest { .isEqualTo(DoSomethingProvider.name); } + @Test + void providerToMultipleNames() { + FeatureProvider inMemAsEventingProvider = new InMemoryProvider(Collections.EMPTY_MAP); + FeatureProvider noOpAsNonEventingProvider = new NoOpProvider(); + + // register same provider for multiple names & as default provider + OpenFeatureAPI.getInstance().setProviderAndWait(inMemAsEventingProvider); + OpenFeatureAPI.getInstance().setProviderAndWait("clientA", inMemAsEventingProvider); + OpenFeatureAPI.getInstance().setProviderAndWait("clientB", inMemAsEventingProvider); + OpenFeatureAPI.getInstance().setProviderAndWait("clientC", noOpAsNonEventingProvider); + OpenFeatureAPI.getInstance().setProviderAndWait("clientD", noOpAsNonEventingProvider); + + assertEquals(inMemAsEventingProvider, OpenFeatureAPI.getInstance().getProvider()); + assertEquals(inMemAsEventingProvider, OpenFeatureAPI.getInstance().getProvider("clientA")); + assertEquals(inMemAsEventingProvider, OpenFeatureAPI.getInstance().getProvider("clientB")); + assertEquals(noOpAsNonEventingProvider, OpenFeatureAPI.getInstance().getProvider("clientC")); + assertEquals(noOpAsNonEventingProvider, OpenFeatureAPI.getInstance().getProvider("clientD")); + } + @Test void settingDefaultProviderToNullErrors() { assertThatCode(() -> api.setProvider(null)).isInstanceOf(IllegalArgumentException.class); diff --git a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java index 5b475dc3..b78d0afb 100644 --- a/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java +++ b/src/test/java/dev/openfeature/sdk/ProviderRepositoryTest.java @@ -9,6 +9,7 @@ import static org.assertj.core.api.Assertions.assertThatCode; import static org.awaitility.Awaitility.await; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.atMostOnce; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; diff --git a/src/test/java/dev/openfeature/sdk/testutils/FeatureProviderTestUtils.java b/src/test/java/dev/openfeature/sdk/testutils/FeatureProviderTestUtils.java index 5f8c13db..b3fc898e 100644 --- a/src/test/java/dev/openfeature/sdk/testutils/FeatureProviderTestUtils.java +++ b/src/test/java/dev/openfeature/sdk/testutils/FeatureProviderTestUtils.java @@ -8,6 +8,7 @@ import lombok.experimental.UtilityClass; import static org.awaitility.Awaitility.await; +// todo check the need of this utility class as we now have setProviderAndWait capability @UtilityClass public class FeatureProviderTestUtils {