add tests

Signed-off-by: Todd Baert <[email protected]>

Author: toddbaert

pom.xml

@@ -164,6 +164,21 @@
   <build>
     <plugins>
 
+      <plugin>
+        <groupId>org.codehaus.mojo</groupId>
+        <artifactId>build-helper-maven-plugin</artifactId>
+        <version>3.3.0</version>
+        <executions>
+          <execution>
+            <phase>validate</phase>
+            <id>get-cpu-count</id>
+            <goals>
+              <goal>cpu-count</goal>
+            </goals>
+          </execution>
+        </executions>
+      </plugin>
+
       <plugin>
         <groupId>org.cyclonedx</groupId>
         <artifactId>cyclonedx-maven-plugin</artifactId>
@@ -231,6 +246,9 @@
           <argLine>
             ${surefireArgLine}
           </argLine>
+          <!-- fork a new JVM to isolate test suites, especially import with singletons -->
+          <forkCount>${cpu.count}</forkCount>
+          <reuseForks>false</reuseForks>
           <excludes>
             <!-- tests to exclude -->
             <exclude>${testExclusions}</exclude>

spotbugs-exclusions.xml

@@ -9,11 +9,16 @@
         <Class name="dev.openfeature.sdk.OpenFeatureAPI"/>
         <Bug pattern="MS_EXPOSE_REP"/>
     </And>
-    <!-- similarly, client using the singleton doesn't seem bad -->
+    <!-- evaluation context is mutable if mutable impl is used -->
     <And>
         <Class name="dev.openfeature.sdk.OpenFeatureClient"/>
         <Bug pattern="EI_EXPOSE_REP2"/>
     </And>
+    <And>
+        <Class name="dev.openfeature.sdk.OpenFeatureAPI"/>
+        <Bug pattern="EI_EXPOSE_REP2"/>
+    </And>
+
 
     <!-- Test class that should be excluded -->
     <Match>

src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java

@@ -9,7 +9,6 @@
 import dev.openfeature.sdk.internal.AutoCloseableLock;
 import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock;
 import lombok.Getter;
-import lombok.Setter;
 
 /**
  * A global singleton which holds base configuration for the OpenFeature library.
@@ -21,7 +20,6 @@ public class OpenFeatureAPI {
     @Getter
     private FeatureProvider provider;
     @Getter
-    @Setter
     private EvaluationContext evaluationContext;
     @Getter
     private List<Hook> apiHooks;
@@ -58,6 +56,15 @@ public Client getClient(@Nullable String name, @Nullable String version) {
         return new OpenFeatureClient(this, name, version);
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    public void setEvaluationContext(EvaluationContext evaluationContext) {
+        try (AutoCloseableLock __ = rwLock.writeLockAutoCloseable()) {
+            this.evaluationContext = evaluationContext;
+        }
+    }
+
     /**
      * {@inheritDoc}
      */

src/main/java/dev/openfeature/sdk/OpenFeatureClient.java

@@ -12,7 +12,6 @@
 import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock;
 import dev.openfeature.sdk.internal.ObjectUtils;
 import lombok.Getter;
-import lombok.Setter;
 import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
@@ -27,10 +26,9 @@ public class OpenFeatureClient implements Client {
     @Getter
     private final List<Hook> clientHooks;
     private final HookSupport hookSupport;
-    private AutoCloseableReentrantReadWriteLock rwLock = new AutoCloseableReentrantReadWriteLock();
+    AutoCloseableReentrantReadWriteLock rwLock = new AutoCloseableReentrantReadWriteLock();
 
     @Getter
-    @Setter
     private EvaluationContext evaluationContext;
 
     /**
@@ -49,13 +47,26 @@ public OpenFeatureClient(OpenFeatureAPI openFeatureAPI, String name, String vers
         this.hookSupport = new HookSupport();
     }
 
+    /**
+     * {@inheritDoc}
+     */
     @Override
     public void addHooks(Hook... hooks) {
         try (AutoCloseableLock __ = this.rwLock.writeLockAutoCloseable()) {
             this.clientHooks.addAll(Arrays.asList(hooks));
         }
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public void setEvaluationContext(EvaluationContext evaluationContext) {
+        try (AutoCloseableLock __ = rwLock.writeLockAutoCloseable()) {
+            this.evaluationContext = evaluationContext;
+        }
+    }
+
     private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key, T defaultValue,
             EvaluationContext ctx, FlagEvaluationOptions options) {
         FlagEvaluationOptions flagOptions = ObjectUtils.defaultIfNull(options,
@@ -74,6 +85,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
             EvaluationContext clientContext;
 
             // lock while getting the provider and hooks
+            // the retrieval any mutable state on client/API MUST be done in this block.
             try (AutoCloseableLock __ = OpenFeatureAPI.rwLock.readLockAutoCloseable();
                     AutoCloseableLock ___ = this.rwLock.readLockAutoCloseable()) {
                 provider = ObjectUtils.defaultIfNull(openfeatureApi.getProvider(), () -> {

src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java

@@ -6,12 +6,14 @@
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
+import java.util.Arrays;
+import java.util.Map;
+import java.util.Optional;
+
 import org.junit.jupiter.api.Test;
 
 import dev.openfeature.sdk.fixtures.HookFixtures;
 
-import java.util.Arrays;
-
 class DeveloperExperienceTest implements HookFixtures {
     transient String flagKey = "mykey";
 
@@ -90,4 +92,34 @@ class DeveloperExperienceTest implements HookFixtures {
         assertEquals(Reason.ERROR.toString(), retval.getReason());
         assertFalse(retval.getValue());
     }
+
+    @Test
+    void providerLockedPerTransaction() throws InterruptedException {
+
+        class MutatingHook implements Hook {
+
+            @Override
+            // change the provider during a before hook - this should not impact the evaluation in progress
+            public Optional before(HookContext ctx, Map hints) {
+                OpenFeatureAPI.getInstance().setProvider(new NoOpProvider());
+                return Optional.empty();
+            }
+        }
+        
+        final String defaultValue = "string-value";
+        final OpenFeatureAPI api = OpenFeatureAPI.getInstance();
+        final Client client = api.getClient();
+        api.setProvider(new DoSomethingProvider());
+        api.addHooks(new MutatingHook());
+
+        // if provider is changed during an evaluation transaction it should proceed with the original provider
+        String doSomethingValue = client.getStringValue("val", defaultValue);
+        assertEquals(new StringBuilder(defaultValue).reverse().toString(), doSomethingValue);
+
+        api.clearHooks();
+        
+        // subsequent evaluations should now use new provider set by hook
+        String noOpValue = client.getStringValue("val", defaultValue);
+        assertEquals(noOpValue, defaultValue);
+    }
 }

src/test/java/dev/openfeature/sdk/DoSomethingProvider.java

@@ -2,6 +2,7 @@
 
 public class DoSomethingProvider implements FeatureProvider {
 
+    public static final String name = "Something";
     private EvaluationContext savedContext;
 
     public EvaluationContext getMergedContext() {
@@ -10,7 +11,7 @@ public EvaluationContext getMergedContext() {
 
     @Override
     public Metadata getMetadata() {
-        return () -> "test";
+        return () -> name;
     }
 
     @Override

src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java

@@ -53,7 +53,7 @@ private Client _client() {
     @Test void provider_metadata() {
         OpenFeatureAPI api = OpenFeatureAPI.getInstance();
         api.setProvider(new DoSomethingProvider());
-        assertEquals("test", api.getProviderMetadata().getName());
+        assertEquals(DoSomethingProvider.name, api.getProviderMetadata().getName());
     }
 
     @Specification(number="1.1.3", text="The API MUST provide a function to add hooks which accepts one or more API-conformant hooks, and appends them to the collection of any previously added hooks. When new hooks are added, previously added hooks are not removed.")

src/test/java/dev/openfeature/sdk/LockingTest.java

@@ -0,0 +1,117 @@
+package dev.openfeature.sdk;
+
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock;
+
+class LockingTest {
+
+    private static OpenFeatureAPI api;
+    private OpenFeatureClient client;
+    private AutoCloseableReentrantReadWriteLock apiRwLock;
+    private AutoCloseableReentrantReadWriteLock clientRwLock;
+    private ReentrantReadWriteLock.ReadLock mockClientReadLock;
+    private ReentrantReadWriteLock.WriteLock mockClientWriteLock;
+    private ReentrantReadWriteLock.ReadLock mockApiReadLock;
+    private ReentrantReadWriteLock.WriteLock mockApiWriteLock;
+
+    @BeforeAll
+    static void beforeAll() {
+        api = OpenFeatureAPI.getInstance();
+    }
+
+    @BeforeEach
+    void beforeEach() {
+        client = (OpenFeatureClient)api.getClient();
+
+        // mock the inner read and write locks
+        mockClientReadLock = mockInnerReadLock();
+        mockClientWriteLock = mockInnerWriteLock();
+        mockApiReadLock = mockInnerReadLock();
+        mockApiWriteLock = mockInnerWriteLock();
+
+        // mock the client rwLock
+        clientRwLock = mock(AutoCloseableReentrantReadWriteLock.class);
+        when(clientRwLock.readLockAutoCloseable()).thenCallRealMethod();
+        when(clientRwLock.readLock()).thenReturn(mockClientReadLock);
+        when(clientRwLock.writeLockAutoCloseable()).thenCallRealMethod();
+        when(clientRwLock.writeLock()).thenReturn(mockClientWriteLock);
+        client.rwLock = clientRwLock;
+
+        // mock the API rwLock
+        apiRwLock = mock(AutoCloseableReentrantReadWriteLock.class);
+        when(apiRwLock.readLockAutoCloseable()).thenCallRealMethod();
+        when(apiRwLock.readLock()).thenReturn(mockApiReadLock);
+        when(apiRwLock.writeLockAutoCloseable()).thenCallRealMethod();
+        when(apiRwLock.writeLock()).thenReturn(mockApiWriteLock);
+        OpenFeatureAPI.rwLock = apiRwLock;
+    }
+
+    @Test
+    void evaluationShouldReadLockandReadUnlockClientAndApi() {
+        client.getBooleanValue("a-key", false);
+        verify(mockApiReadLock).lock();
+        verify(mockApiReadLock).unlock();
+        verify(mockClientReadLock).lock();
+        verify(mockClientReadLock).unlock();
+    }
+
+    @Test
+    void addHooksShouldWriteLockAndUnlock() {
+        client.addHooks(new Hook(){});
+        verify(mockClientWriteLock).lock();
+        verify(mockClientWriteLock).unlock();
+
+        api.addHooks(new Hook(){});
+        verify(mockApiWriteLock).lock();
+        verify(mockApiWriteLock).unlock();
+    }
+
+    @Test
+    void setContextShouldWriteLockAndUnlock() {
+        client.setEvaluationContext(new MutableContext());
+        verify(mockClientWriteLock).lock();
+        verify(mockClientWriteLock).unlock();
+
+        api.setEvaluationContext(new MutableContext());
+        verify(mockApiWriteLock).lock();
+        verify(mockApiWriteLock).unlock();
+    }
+
+    @Test
+    void setProviderShouldWriteLockAndUnlock() {
+        api.setProvider(new DoSomethingProvider());
+        verify(mockApiWriteLock).lock();
+        verify(mockApiWriteLock).unlock();
+    }
+
+    @Test
+    void clearHooksShouldWriteLockAndUnlock() {
+        api.clearHooks();
+        verify(mockApiWriteLock).lock();
+        verify(mockApiWriteLock).unlock();
+    }
+
+    private static ReentrantReadWriteLock.ReadLock mockInnerReadLock() {
+        ReentrantReadWriteLock.ReadLock readLockMock = mock(ReentrantReadWriteLock.ReadLock.class);
+        doNothing().when(readLockMock).lock();
+        doNothing().when(readLockMock).unlock();
+        return readLockMock;
+    }
+
+    private static ReentrantReadWriteLock.WriteLock mockInnerWriteLock() {
+        ReentrantReadWriteLock.WriteLock writeLockMock = mock(ReentrantReadWriteLock.WriteLock.class);
+        doNothing().when(writeLockMock).lock();
+        doNothing().when(writeLockMock).unlock();
+        return writeLockMock;
+    }
+}