fix: tolerate duplicate provider registrations (#725)

* fix provider mulitple regiration issue Signed-off-by: Kavindu Dodanduwa <[email protected]> * fix lint Signed-off-by: Kavindu Dodanduwa <[email protected]> * fix tests Signed-off-by: Kavindu Dodanduwa <[email protected]> * improve test and add check for unused imports Signed-off-by: Kavindu Dodanduwa <[email protected]> --------- Signed-off-by: Kavindu Dodanduwa <[email protected]>
open-feature · Dec 12, 2023 · 3319e55 · 3319e55
1 parent 07ea4c0
commit 3319e55
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 10 deletions.
diff --git a/checkstyle.xml b/checkstyle.xml
@@ -63,7 +63,8 @@
                         default="checkstyle-xpath-suppressions.xml" />
             <property name="optional" value="true"/>
         </module>
-
+
+        <module name="UnusedImports"/>
         <module name="OuterTypeFilename"/>
         <module name="IllegalTokenText">
             <property name="tokens" value="STRING_LITERAL, CHAR_LITERAL"/>

diff --git 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<EventProvider, ProviderEvent, ProviderEventDetails> 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
@@ -106,11 +106,16 @@ private void prepareAndInitializeProvider(@Nullable String clientName,
               BiConsumer<FeatureProvider, String> 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 @@ private void initializeProvider(FeatureProvider newProvider,
         }
     }
 
-    private void shutDownOld(FeatureProvider oldProvider,Consumer<FeatureProvider> afterShutdown) {
-        if (oldProvider != null && !isProviderRegistered(oldProvider)) {
+    private void shutDownOld(FeatureProvider oldProvider, Consumer<FeatureProvider> 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
@@ -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 @@ void namedProviderOverwrittenTest() {
                 .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
@@ -9,6 +9,7 @@
 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
@@ -8,6 +8,7 @@
 
 import static org.awaitility.Awaitility.await;
 
+// todo check the need of this utility class as we now have setProviderAndWait capability
 @UtilityClass
 public class FeatureProviderTestUtils {