From d4c43d74bc37371fc19dc1983e96e7c904d5a1e7 Mon Sep 17 00:00:00 2001 From: Justin Abrahms Date: Fri, 19 May 2023 15:27:31 -0700 Subject: [PATCH] feat: Support mapping a client to a given provider. (#388) * Support mapping a client to a given provider. Signed-off-by: Justin Abrahms * Add a few javadocs. Signed-off-by: Justin Abrahms * Special case the null client name Signed-off-by: Justin Abrahms * Add some missing test cases. Signed-off-by: Justin Abrahms * Moving to an object map unwraps the values. Signed-off-by: Justin Abrahms * Fix equality test. Signed-off-by: Justin Abrahms * Carry targeting key when copying over null object. Signed-off-by: Justin Abrahms * Test provider name, not object equality. Signed-off-by: Justin Abrahms * Client-based getProvider is now an overload; Use read lock, not write lock. Signed-off-by: Justin Abrahms * Update src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java Co-authored-by: Lars Opitz Signed-off-by: Justin Abrahms * Simplify locking logic around providers. There's no such thing as "API without a provider set" anymore. We now default to NoOpProvider in the API (not client). Signed-off-by: Justin Abrahms * Add a few missing tests Signed-off-by: Justin Abrahms --------- Signed-off-by: Justin Abrahms Co-authored-by: Lars Opitz Co-authored-by: Michael Beemer --- .../dev/openfeature/sdk/MutableContext.java | 4 +- .../dev/openfeature/sdk/OpenFeatureAPI.java | 52 ++++++++++++++----- .../openfeature/sdk/OpenFeatureClient.java | 7 +-- .../sdk/ClientProviderMappingTest.java | 23 ++++++++ .../sdk/DeveloperExperienceTest.java | 9 ---- .../dev/openfeature/sdk/EvalContextTest.java | 7 +++ .../sdk/ImmutableStructureTest.java | 11 ++++ .../java/dev/openfeature/sdk/LockingTest.java | 9 ---- .../openfeature/sdk/OpenFeatureAPITest.java | 26 ++++++++++ .../sdk/OpenFeatureClientTest.java | 4 +- 10 files changed, 113 insertions(+), 39 deletions(-) create mode 100644 src/test/java/dev/openfeature/sdk/ClientProviderMappingTest.java create mode 100644 src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java diff --git a/src/main/java/dev/openfeature/sdk/MutableContext.java b/src/main/java/dev/openfeature/sdk/MutableContext.java index 6abd74a5..9e7069ca 100644 --- a/src/main/java/dev/openfeature/sdk/MutableContext.java +++ b/src/main/java/dev/openfeature/sdk/MutableContext.java @@ -4,6 +4,7 @@ import java.time.Instant; import java.util.List; import java.util.Map; +import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.Setter; import lombok.ToString; @@ -16,6 +17,7 @@ import lombok.experimental.Delegate; * be modified after instantiation. */ @ToString +@EqualsAndHashCode @SuppressWarnings("PMD.BeanMembersShouldSerialize") public class MutableContext implements EvaluationContext { @@ -88,7 +90,7 @@ public class MutableContext implements EvaluationContext { @Override public EvaluationContext merge(EvaluationContext overridingContext) { if (overridingContext == null) { - return new MutableContext(this.asMap()); + return new MutableContext(this.targetingKey, this.asMap()); } Map merged = this.merge(map -> new MutableStructure(map), diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java index e85f4e13..a2ddc453 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java @@ -1,8 +1,7 @@ package dev.openfeature.sdk; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; +import java.util.*; +import java.util.concurrent.ConcurrentHashMap; import javax.annotation.Nullable; @@ -16,11 +15,11 @@ import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock; public class OpenFeatureAPI { // package-private multi-read/single-write lock static AutoCloseableReentrantReadWriteLock hooksLock = new AutoCloseableReentrantReadWriteLock(); - static AutoCloseableReentrantReadWriteLock providerLock = new AutoCloseableReentrantReadWriteLock(); static AutoCloseableReentrantReadWriteLock contextLock = new AutoCloseableReentrantReadWriteLock(); - private FeatureProvider provider; private EvaluationContext evaluationContext; - private List apiHooks; + private final List apiHooks; + private FeatureProvider defaultProvider = new NoOpProvider(); + private final Map providers = new ConcurrentHashMap<>(); private OpenFeatureAPI() { this.apiHooks = new ArrayList<>(); @@ -39,7 +38,11 @@ public class OpenFeatureAPI { } public Metadata getProviderMetadata() { - return provider.getMetadata(); + return defaultProvider.getMetadata(); + } + + public Metadata getProviderMetadata(String clientName) { + return getProvider(clientName).getMetadata(); } public Client getClient() { @@ -73,23 +76,44 @@ public class OpenFeatureAPI { } /** - * {@inheritDoc} + * Set the default provider. */ public void setProvider(FeatureProvider provider) { - try (AutoCloseableLock __ = providerLock.writeLockAutoCloseable()) { - this.provider = provider; + if (provider == null) { + throw new IllegalArgumentException("Provider cannot be null"); } + defaultProvider = provider; } /** - * {@inheritDoc} + * Add a provider for a named client. + * @param clientName The name of the client. + * @param provider The provider to set. + */ + public void setProvider(String clientName, FeatureProvider provider) { + if (provider == null) { + throw new IllegalArgumentException("Provider cannot be null"); + } + this.providers.put(clientName, provider); + } + + /** + * Return the default provider. */ public FeatureProvider getProvider() { - try (AutoCloseableLock __ = providerLock.readLockAutoCloseable()) { - return this.provider; - } + return defaultProvider; } + /** + * Fetch a provider for a named client. If not found, return the default. + * @param name The client name to look for. + * @return A named {@link FeatureProvider} + */ + public FeatureProvider getProvider(String name) { + return Optional.ofNullable(name).map(this.providers::get).orElse(defaultProvider); + } + + /** * {@inheritDoc} */ diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index a13eb942..44febf77 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -99,17 +99,14 @@ public class OpenFeatureClient implements Client { FlagEvaluationDetails details = null; List mergedHooks = null; HookContext hookCtx = null; - FeatureProvider provider = null; + FeatureProvider provider; try { final EvaluationContext apiContext; final EvaluationContext clientContext; // openfeatureApi.getProvider() must be called once to maintain a consistent reference - provider = ObjectUtils.defaultIfNull(openfeatureApi.getProvider(), () -> { - log.debug("No provider configured, using no-op provider."); - return new NoOpProvider(); - }); + provider = openfeatureApi.getProvider(this.name); mergedHooks = ObjectUtils.merge(provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getHooks()); diff --git a/src/test/java/dev/openfeature/sdk/ClientProviderMappingTest.java b/src/test/java/dev/openfeature/sdk/ClientProviderMappingTest.java new file mode 100644 index 00000000..654fb335 --- /dev/null +++ b/src/test/java/dev/openfeature/sdk/ClientProviderMappingTest.java @@ -0,0 +1,23 @@ +package dev.openfeature.sdk; + +import io.cucumber.java.eo.Do; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class ClientProviderMappingTest { + @Test + void clientProviderTest() { + OpenFeatureAPI api = OpenFeatureAPI.getInstance(); + + api.setProvider("client1", new DoSomethingProvider()); + api.setProvider("client2", new NoOpProvider()); + + Client c1 = api.getClient("client1"); + Client c2 = api.getClient("client2"); + + assertTrue(c1.getBooleanValue("test", false)); + assertFalse(c2.getBooleanValue("test", false)); + } +} diff --git a/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java b/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java index f32711a2..600dc7a6 100644 --- a/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java +++ b/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java @@ -19,15 +19,6 @@ import dev.openfeature.sdk.fixtures.HookFixtures; class DeveloperExperienceTest implements HookFixtures { transient String flagKey = "mykey"; - @Test void noProviderSet() { - final String noOp = "no-op"; - OpenFeatureAPI api = OpenFeatureAPI.getInstance(); - api.setProvider(null); - Client client = api.getClient(); - String retval = client.getStringValue(flagKey, noOp); - assertEquals(noOp, retval); - } - @Test void simpleBooleanFlag() { OpenFeatureAPI api = OpenFeatureAPI.getInstance(); api.setProvider(new NoOpProvider()); diff --git a/src/test/java/dev/openfeature/sdk/EvalContextTest.java b/src/test/java/dev/openfeature/sdk/EvalContextTest.java index 29fd0898..f4cd804c 100644 --- a/src/test/java/dev/openfeature/sdk/EvalContextTest.java +++ b/src/test/java/dev/openfeature/sdk/EvalContextTest.java @@ -162,6 +162,13 @@ public class EvalContextTest { assertEquals(key1, ctxMerged.getTargetingKey()); } + @Test void merge_null_returns_value() { + MutableContext ctx1 = new MutableContext("key"); + ctx1.add("mything", "value"); + EvaluationContext result = ctx1.merge(null); + assertEquals(ctx1, result); + } + @Test void merge_targeting_key() { String key1 = "key1"; MutableContext ctx1 = new MutableContext(key1); diff --git a/src/test/java/dev/openfeature/sdk/ImmutableStructureTest.java b/src/test/java/dev/openfeature/sdk/ImmutableStructureTest.java index 49cd236a..d7453452 100644 --- a/src/test/java/dev/openfeature/sdk/ImmutableStructureTest.java +++ b/src/test/java/dev/openfeature/sdk/ImmutableStructureTest.java @@ -111,4 +111,15 @@ class ImmutableStructureTest { Object value = structure.getValue("missing"); assertNull(value); } + + @Test void objectMapTest() { + Map attrs = new HashMap<>(); + attrs.put("test", new Value(45)); + ImmutableStructure structure = new ImmutableStructure(attrs); + + Map expected = new HashMap<>(); + expected.put("test", 45); + + assertEquals(expected, structure.asObjectMap()); + } } diff --git a/src/test/java/dev/openfeature/sdk/LockingTest.java b/src/test/java/dev/openfeature/sdk/LockingTest.java index f8dceee6..3d8d90c8 100644 --- a/src/test/java/dev/openfeature/sdk/LockingTest.java +++ b/src/test/java/dev/openfeature/sdk/LockingTest.java @@ -19,7 +19,6 @@ class LockingTest { private OpenFeatureClient client; private AutoCloseableReentrantReadWriteLock apiContextLock; private AutoCloseableReentrantReadWriteLock apiHooksLock; - private AutoCloseableReentrantReadWriteLock apiProviderLock; private AutoCloseableReentrantReadWriteLock clientContextLock; private AutoCloseableReentrantReadWriteLock clientHooksLock; @@ -33,10 +32,8 @@ class LockingTest { client = (OpenFeatureClient) api.getClient(); apiContextLock = setupLock(apiContextLock, mockInnerReadLock(), mockInnerWriteLock()); - apiProviderLock = setupLock(apiProviderLock, mockInnerReadLock(), mockInnerWriteLock()); apiHooksLock = setupLock(apiHooksLock, mockInnerReadLock(), mockInnerWriteLock()); OpenFeatureAPI.contextLock = apiContextLock; - OpenFeatureAPI.providerLock = apiProviderLock; OpenFeatureAPI.hooksLock = apiHooksLock; clientContextLock = setupLock(clientContextLock, mockInnerReadLock(), mockInnerWriteLock()); @@ -91,12 +88,6 @@ class LockingTest { verify(apiContextLock.readLock()).unlock(); } - @Test - void setProviderShouldWriteLockAndUnlock() { - api.setProvider(new DoSomethingProvider()); - verify(apiProviderLock.writeLock()).lock(); - verify(apiProviderLock.writeLock()).unlock(); - } @Test void clearHooksShouldWriteLockAndUnlock() { diff --git a/src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java b/src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java new file mode 100644 index 00000000..4428f9ff --- /dev/null +++ b/src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java @@ -0,0 +1,26 @@ +package dev.openfeature.sdk; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class OpenFeatureAPITest { + @Test + void namedProviderTest() { + OpenFeatureAPI api = OpenFeatureAPI.getInstance(); + FeatureProvider provider = new NoOpProvider(); + api.setProvider("namedProviderTest", provider); + assertEquals(provider.getMetadata().getName(), api.getProviderMetadata("namedProviderTest").getName()); + } + + @Test void settingDefaultProviderToNullErrors() { + OpenFeatureAPI api = OpenFeatureAPI.getInstance(); + assertThrows(IllegalArgumentException.class, () -> api.setProvider(null)); + } + + @Test void settingNamedClientProviderToNullErrors() { + OpenFeatureAPI api = OpenFeatureAPI.getInstance(); + assertThrows(IllegalArgumentException.class, () -> api.setProvider("client-name", null)); + } +} diff --git a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java index ac150677..9036576d 100644 --- a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java +++ b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java @@ -27,7 +27,7 @@ class OpenFeatureClientTest implements HookFixtures { @DisplayName("should not throw exception if hook has different type argument than hookContext") void shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext() { OpenFeatureAPI api = mock(OpenFeatureAPI.class); - when(api.getProvider()).thenReturn(new DoSomethingProvider()); + when(api.getProvider(any())).thenReturn(new DoSomethingProvider()); when(api.getHooks()).thenReturn(Arrays.asList(mockBooleanHook(), mockStringHook())); OpenFeatureClient client = new OpenFeatureClient(api, "name", "version"); @@ -57,6 +57,8 @@ class OpenFeatureClientTest implements HookFixtures { context -> context.getTargetingKey().equals(targetingKey)))).thenReturn(ProviderEvaluation.builder() .value(true).build()); when(api.getProvider()).thenReturn(mockProvider); + when(api.getProvider(any())).thenReturn(mockProvider); + OpenFeatureClient client = new OpenFeatureClient(api, "name", "version"); client.setEvaluationContext(ctx);