fix: Use ConcurrentHashMap for InMemoryProvider (#1057)

* fix: Use ConcurrentHashMap for InMemoryProvider Signed-off-by: Ryan Prayogo <[email protected]> * fix: make the flags field variable final Signed-off-by: Ryan Prayogo <[email protected]> * chore: Use Collections.singletonList instead of Arrays.asList Signed-off-by: Ryan Prayogo <[email protected]> * chore: Update javadoc and parameter name Signed-off-by: Ryan Prayogo <[email protected]> * fixup: await verify Signed-off-by: Todd Baert <[email protected]> * fixup: remove test, key diffing Signed-off-by: Todd Baert <[email protected]> --------- Signed-off-by: Ryan Prayogo <[email protected]> Signed-off-by: Todd Baert <[email protected]> Co-authored-by: Todd Baert <[email protected]> Co-authored-by: Michael Beemer <[email protected]>
open-feature · Aug 22, 2024 · b7ed041 · b7ed041
1 parent a81957f
commit b7ed041
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 25 deletions.
diff --git a/src/main/java/dev/openfeature/sdk/providers/memory/InMemoryProvider.java b/src/main/java/dev/openfeature/sdk/providers/memory/InMemoryProvider.java
@@ -1,12 +1,5 @@
 package dev.openfeature.sdk.providers.memory;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-import java.util.Set;
-
 import dev.openfeature.sdk.EvaluationContext;
 import dev.openfeature.sdk.EventProvider;
 import dev.openfeature.sdk.Metadata;
@@ -24,6 +17,13 @@
 import lombok.SneakyThrows;
 import lombok.extern.slf4j.Slf4j;
 
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
 /**
  * In-memory provider.
  */
@@ -33,7 +33,7 @@ public class InMemoryProvider extends EventProvider {
     @Getter
     private static final String NAME = "InMemoryProvider";
 
-    private Map<String, Flag<?>> flags;
+    private final Map<String, Flag<?>> flags;
 
     @Getter
     private ProviderState state = ProviderState.NOT_READY;
@@ -44,11 +44,11 @@ public Metadata getMetadata() {
     }
 
     public InMemoryProvider(Map<String, Flag<?>> flags) {
-        this.flags = new HashMap<>(flags);
+        this.flags = new ConcurrentHashMap<>(flags);
     }
 
     /**
-     * Initialize the provider.
+     * Initializes the provider.
      * @param evaluationContext evaluation context
      * @throws Exception on error
      */
@@ -60,14 +60,15 @@ public void initialize(EvaluationContext evaluationContext) throws Exception {
     }
 
     /**
-     * Updating provider flags configuration, replacing existing flags.
-     * @param flags the flags to use instead of the previous flags.
+     * Updates the provider flags configuration.
+     * For existing flags, the new configurations replace the old one.
+     * For new flags, they are added to the configuration.
+     * @param newFlags the new flag configurations
      */
-    public void updateFlags(Map<String, Flag<?>> flags) {
-        Set<String> flagsChanged = new HashSet<>();
-        flagsChanged.addAll(this.flags.keySet());
-        flagsChanged.addAll(flags.keySet());
-        this.flags = new HashMap<>(flags);
+    public void updateFlags(Map<String, Flag<?>> newFlags) {
+        Set<String> flagsChanged = new HashSet<>(newFlags.keySet());
+        this.flags.putAll(newFlags);
+
         ProviderEventDetails details = ProviderEventDetails.builder()
             .flagsChanged(new ArrayList<>(flagsChanged))
             .message("flags changed")
@@ -76,13 +77,15 @@ public void updateFlags(Map<String, Flag<?>> flags) {
     }
 
     /**
-     * Updating provider flags configuration with adding or updating a flag.
-     * @param flag the flag to update. If a flag with this key already exists, new flag replaces it.
+     * Updates a single provider flag configuration.
+     * For existing flag, the new configuration replaces the old one.
+     * For new flag, they are added to the configuration.
+     * @param newFlag the flag to update
      */
-    public void updateFlag(String flagKey, Flag<?> flag) {
-        this.flags.put(flagKey, flag);
+    public void updateFlag(String flagKey, Flag<?> newFlag) {
+        this.flags.put(flagKey, newFlag);
         ProviderEventDetails details = ProviderEventDetails.builder()
-            .flagsChanged(Arrays.asList(flagKey))
+            .flagsChanged(Collections.singletonList(flagKey))
             .message("flag added/updated")
             .build();
         emitProviderConfigurationChanged(details);

diff --git a/src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java b/src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java
@@ -2,25 +2,30 @@
 
 import com.google.common.collect.ImmutableMap;
 import dev.openfeature.sdk.Client;
+import dev.openfeature.sdk.EventDetails;
 import dev.openfeature.sdk.ImmutableContext;
 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.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.function.Consumer;
 
 import static dev.openfeature.sdk.Structure.mapToStructure;
 import static dev.openfeature.sdk.testutils.TestFlagsUtils.buildFlags;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.awaitility.Awaitility.await;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -32,8 +37,8 @@ class InMemoryProviderTest {
     private static InMemoryProvider provider;
 
     @SneakyThrows
-    @BeforeAll
-    static void beforeAll() {
+    @BeforeEach
+    void beforeEach() {
         Map<String, Flag<?>> flags = buildFlags();
         provider = spy(new InMemoryProvider(flags));
         OpenFeatureAPI.getInstance().onProviderConfigurationChanged(eventDetails -> {});
@@ -105,4 +110,19 @@ void shouldThrowIfNotInitialized() {
         // ErrorCode.PROVIDER_NOT_READY should be returned when evaluated via the client
         assertThrows(ProviderNotReadyError.class, ()-> inMemoryProvider.getBooleanEvaluation("fail_not_initialized", false, new ImmutableContext()));
     }
+
+    @SuppressWarnings("unchecked")
+    @Test
+    void emitChangedFlagsOnlyIfThereAreChangedFlags() {
+        Consumer<EventDetails> handler = mock(Consumer.class);
+        Map<String, Flag<?>> flags = buildFlags();
+
+        OpenFeatureAPI.getInstance().onProviderConfigurationChanged(handler);
+        OpenFeatureAPI.getInstance().setProviderAndWait(provider);
+
+        provider.updateFlags(flags);
+
+        await().untilAsserted(() -> verify(handler, times(1))
+                .accept(argThat(details -> details.getFlagsChanged().size() == buildFlags().size())));
+    }
 }