Skip to content

Commit 45e9ba2

Browse files
toddbaerttoddbaert
committed
fixup: feedback from guido
Signed-off-by: Todd Baert <[email protected]>
1 parent cd56f25 commit 45e9ba2

File tree

8 files changed

+58
-122
lines changed

8 files changed

+58
-122
lines changed

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

+5-23
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ public Client getClient(String domain) {
103103
*/
104104
public Client getClient(String domain, String version) {
105105
return new OpenFeatureClient(
106-
() -> providerRepository.getFeatureProviderStateManager(domain),
107106
this,
108107
domain,
109108
version
@@ -285,28 +284,6 @@ public FeatureProvider getProvider(String domain) {
285284
return providerRepository.getProvider(domain);
286285
}
287286

288-
/**
289-
* Return the state of the default provider.
290-
*/
291-
public ProviderState getProviderState() {
292-
return providerRepository.getProviderState();
293-
}
294-
295-
/**
296-
* Get the state of the provider for a domain. If no provider with the domain is found, returns the state of the
297-
* default provider.
298-
*
299-
* @param domain The domain to look for.
300-
* @return A named {@link FeatureProvider}
301-
*/
302-
public ProviderState getProviderState(String domain) {
303-
return providerRepository.getProviderState(domain);
304-
}
305-
306-
public ProviderState getProviderState(FeatureProvider provider) {
307-
return providerRepository.getProviderState(provider);
308-
}
309-
310287
/**
311288
* Adds hooks for globally, used for all evaluations.
312289
* Hooks are run in the order they're added in the before stage. They are run in reverse order for all other stages.
@@ -424,6 +401,11 @@ void addHandler(String domain, ProviderEvent event, Consumer<EventDetails> handl
424401
}
425402
}
426403

404+
FeatureProviderStateManager getFeatureProviderStateManager(String domain) {
405+
return providerRepository.getFeatureProviderStateManager(domain);
406+
}
407+
408+
427409
/**
428410
* Runs the handlers associated with a particular provider.
429411
*

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

+3-6
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
@Deprecated() // TODO: eventually we will make this non-public. See issue #872
2525
public class OpenFeatureClient implements Client {
2626

27-
private final ProviderAccessor providerAccessor;
2827
private final OpenFeatureAPI openfeatureApi;
2928
@Getter
3029
private final String domain;
@@ -48,12 +47,10 @@ public class OpenFeatureClient implements Client {
4847
*/
4948
@Deprecated() // TODO: eventually we will make this non-public. See issue #872
5049
public OpenFeatureClient(
51-
ProviderAccessor providerAccessor,
5250
OpenFeatureAPI openFeatureAPI,
5351
String domain,
5452
String version
5553
) {
56-
this.providerAccessor = providerAccessor;
5754
this.openfeatureApi = openFeatureAPI;
5855
this.domain = domain;
5956
this.version = version;
@@ -63,7 +60,7 @@ public OpenFeatureClient(
6360

6461
@Override
6562
public ProviderState getProviderState() {
66-
return providerAccessor.getProviderStateManager().getState();
63+
return openfeatureApi.getFeatureProviderStateManager(domain).getState();
6764
}
6865

6966
/**
@@ -121,8 +118,8 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
121118
FeatureProvider provider;
122119

123120
try {
124-
// providerAccessor.getProviderStateManager() must be called once to maintain a consistent reference
125-
FeatureProviderStateManager stateManager = providerAccessor.getProviderStateManager();
121+
FeatureProviderStateManager stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain);
122+
// provider must be accessed once to maintain a consistent reference
126123
provider = stateManager.getProvider();
127124
ProviderState state = stateManager.getState();
128125
if (ProviderState.NOT_READY.equals(state)) {

‎src/main/java/dev/openfeature/sdk/ProviderAccessor.java

-9
This file was deleted.

‎src/test/java/dev/openfeature/sdk/EventsTest.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -535,12 +535,12 @@ void matchingStaleEventsMustRunImmediately() {
535535

536536
// provider which is already stale
537537
TestEventsProvider provider = TestEventsProvider.newInitializedTestEventsProvider();
538+
Client client = api.getClient(name);
538539
api.setProviderAndWait(name, provider);
539540
provider.emitProviderStale(ProviderEventDetails.builder().build());
540-
assertThat(api.getProviderState(name)).isEqualTo(ProviderState.STALE);
541+
assertThat(client.getProviderState()).isEqualTo(ProviderState.STALE);
541542

542543
// should run even thought handler was added after stale
543-
Client client = api.getClient(name);
544544
client.onProviderStale(handler);
545545
verify(handler, timeout(TIMEOUT)).accept(any());
546546
}
@@ -555,13 +555,13 @@ void matchingErrorEventsMustRunImmediately() {
555555

556556
// provider which is already in error
557557
TestEventsProvider provider = new TestEventsProvider();
558+
Client client = api.getClient(name);
558559
api.setProviderAndWait(name, provider);
559560
provider.emitProviderError(ProviderEventDetails.builder().build());
560-
assertThat(api.getProviderState(name)).isEqualTo(ProviderState.ERROR);
561+
assertThat(client.getProviderState()).isEqualTo(ProviderState.ERROR);
561562

562563

563564
// should run even thought handler was added after error
564-
Client client = api.getClient(name);
565565
client.onProviderError(handler);
566566
verify(handler, timeout(TIMEOUT)).accept(any());
567567
}

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

+4-3
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,14 @@ void getApiInstance() {
7878
@Test void providerAndWait() {
7979
FeatureProvider provider = new TestEventsProvider(500);
8080
OpenFeatureAPI.getInstance().setProviderAndWait(provider);
81-
assertThat(api.getProviderState()).isEqualTo(ProviderState.READY);
81+
Client client = api.getClient();
82+
assertThat(client.getProviderState()).isEqualTo(ProviderState.READY);
8283

8384
provider = new TestEventsProvider(500);
8485
String providerName = "providerAndWait";
8586
OpenFeatureAPI.getInstance().setProviderAndWait(providerName, provider);
86-
assertThat(api.getProviderState(providerName)).isEqualTo(ProviderState.READY);
87+
Client client2 = api.getClient(providerName);
88+
assertThat(client2.getProviderState()).isEqualTo(ProviderState.READY);
8789
}
8890

8991
@SneakyThrows
@@ -102,7 +104,6 @@ void getApiInstance() {
102104
FeatureProvider provider = new TestEventsProvider(100);
103105
String providerName = "shouldReturnNotReadyIfNotInitialized";
104106
OpenFeatureAPI.getInstance().setProvider(providerName, provider);
105-
assertThat(api.getProviderState(providerName)).isEqualTo(ProviderState.NOT_READY);
106107
Client client = OpenFeatureAPI.getInstance().getClient(providerName);
107108
FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("return_error_when_not_initialized", false);
108109
assertEquals(ErrorCode.PROVIDER_NOT_READY, details.getErrorCode());

‎src/test/java/dev/openfeature/sdk/OpenFeatureAPITest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ void settingTransactionalContextPropagatorToNullErrors() {
8282

8383
@Test
8484
void setEvaluationContextShouldAllowChaining() {
85-
OpenFeatureClient client = new OpenFeatureClient(() -> null, api, "name", "version");
85+
OpenFeatureClient client = new OpenFeatureClient(api, "name", "version");
8686
EvaluationContext ctx = new ImmutableContext("targeting key", new HashMap<>());
8787
OpenFeatureClient result = client.setEvaluationContext(ctx);
8888
assertEquals(client, result);

‎src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java

+29-76
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,16 @@
11
package dev.openfeature.sdk;
22

3-
import dev.openfeature.sdk.fixtures.HookFixtures;
4-
import lombok.SneakyThrows;
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.junit.jupiter.api.Assertions.assertEquals;
5+
import static org.junit.jupiter.api.Assertions.assertThrows;
6+
import static org.mockito.ArgumentMatchers.any;
7+
import static org.mockito.ArgumentMatchers.anyString;
8+
import static org.mockito.Mockito.mock;
9+
import static org.mockito.Mockito.never;
10+
11+
import java.util.HashMap;
12+
import java.util.concurrent.atomic.AtomicBoolean;
13+
514
import org.junit.jupiter.api.AfterEach;
615
import org.junit.jupiter.api.BeforeEach;
716
import org.junit.jupiter.api.DisplayName;
@@ -10,14 +19,9 @@
1019
import org.simplify4u.slf4jmock.LoggerMock;
1120
import org.slf4j.Logger;
1221

13-
import java.util.Arrays;
14-
import java.util.HashMap;
15-
import java.util.concurrent.atomic.AtomicBoolean;
16-
17-
import static org.assertj.core.api.Assertions.assertThat;
18-
import static org.junit.jupiter.api.Assertions.assertEquals;
19-
import static org.mockito.ArgumentMatchers.*;
20-
import static org.mockito.Mockito.*;
22+
import dev.openfeature.sdk.exceptions.FatalError;
23+
import dev.openfeature.sdk.fixtures.HookFixtures;
24+
import dev.openfeature.sdk.testutils.TestEventsProvider;
2125

2226
class OpenFeatureClientTest implements HookFixtures {
2327

@@ -37,14 +41,10 @@ void reset_logs() {
3741
@Test
3842
@DisplayName("should not throw exception if hook has different type argument than hookContext")
3943
void shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext() {
40-
DoSomethingProvider provider = new DoSomethingProvider();
41-
OpenFeatureAPI api = mock(OpenFeatureAPI.class);
42-
when(api.getProvider(any())).thenReturn(provider);
43-
when(api.getHooks()).thenReturn(Arrays.asList(mockBooleanHook(), mockStringHook()));
44-
45-
MockProviderRepository mockProviderRepository = new MockProviderRepository(provider, true);
46-
OpenFeatureClient client = new OpenFeatureClient(mockProviderRepository, api, "name", "version");
47-
44+
OpenFeatureAPI api = OpenFeatureAPI.getInstance();
45+
api.setProvider("shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext", new DoSomethingProvider());
46+
Client client = api.getClient("shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext");
47+
client.addHooks(mockBooleanHook(), mockStringHook());
4848
FlagEvaluationDetails<Boolean> actual = client.getBooleanDetails("feature key", Boolean.FALSE);
4949

5050
assertThat(actual.getValue()).isTrue();
@@ -56,36 +56,11 @@ void shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext() {
5656
Mockito.verify(logger, never()).error(anyString(), any(), any());
5757
}
5858

59-
@Test
60-
void mergeContextTest() {
61-
String flag = "feature key";
62-
boolean defaultValue = false;
63-
String targetingKey = "targeting key";
64-
EvaluationContext ctx = new ImmutableContext(targetingKey, new HashMap<>());
65-
OpenFeatureAPI api = mock(OpenFeatureAPI.class);
66-
FeatureProvider mockProvider = mock(FeatureProvider.class);
67-
// this makes it so that true is returned only if the targeting key set at the client level is honored
68-
when(mockProvider.getBooleanEvaluation(
69-
eq(flag), eq(defaultValue), argThat(
70-
context -> context.getTargetingKey().equals(targetingKey)))).thenReturn(ProviderEvaluation.<Boolean>builder()
71-
.value(true).build());
72-
when(api.getProvider()).thenReturn(mockProvider);
73-
when(api.getProvider(any())).thenReturn(mockProvider);
74-
75-
MockProviderRepository mockProviderRepository = new MockProviderRepository(mockProvider, true);
76-
OpenFeatureClient client = new OpenFeatureClient(mockProviderRepository, api, "name", "version");
77-
client.setEvaluationContext(ctx);
78-
79-
FlagEvaluationDetails<Boolean> result = client.getBooleanDetails(flag, defaultValue);
80-
81-
assertThat(result.getValue()).isTrue();
82-
}
83-
8459
@Test
8560
@DisplayName("addHooks should allow chaining by returning the same client instance")
8661
void addHooksShouldAllowChaining() {
8762
OpenFeatureAPI api = mock(OpenFeatureAPI.class);
88-
OpenFeatureClient client = new OpenFeatureClient(() -> null, api, "name", "version");
63+
OpenFeatureClient client = new OpenFeatureClient(api, "name", "version");
8964
Hook<?> hook1 = Mockito.mock(Hook.class);
9065
Hook<?> hook2 = Mockito.mock(Hook.class);
9166

@@ -97,7 +72,7 @@ void addHooksShouldAllowChaining() {
9772
@DisplayName("setEvaluationContext should allow chaining by returning the same client instance")
9873
void setEvaluationContextShouldAllowChaining() {
9974
OpenFeatureAPI api = mock(OpenFeatureAPI.class);
100-
OpenFeatureClient client = new OpenFeatureClient(() -> null, api, "name", "version");
75+
OpenFeatureClient client = new OpenFeatureClient(api, "name", "version");
10176
EvaluationContext ctx = new ImmutableContext("targeting key", new HashMap<>());
10277

10378
OpenFeatureClient result = client.setEvaluationContext(ctx);
@@ -107,49 +82,27 @@ void setEvaluationContextShouldAllowChaining() {
10782
@Test
10883
@DisplayName("Should not call evaluation methods when the provider has state FATAL")
10984
void shouldNotCallEvaluationMethodsWhenProviderIsInFatalErrorState() {
110-
MockProvider mockProvider = new MockProvider(ProviderState.FATAL);
111-
OpenFeatureAPI api = mock(OpenFeatureAPI.class);
112-
MockProviderRepository mockProviderRepository = new MockProviderRepository(mockProvider, true);
113-
OpenFeatureClient client = new OpenFeatureClient(mockProviderRepository, api, "name", "version");
114-
mockProviderRepository.featureProviderStateManager.onEmit(
115-
ProviderEvent.PROVIDER_ERROR,
116-
ProviderEventDetails.builder().errorCode(ErrorCode.PROVIDER_FATAL).build()
117-
);
118-
FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("key", true);
85+
FeatureProvider provider = new TestEventsProvider(100, true, "fake fatal", true);
86+
OpenFeatureAPI api = OpenFeatureAPI.getInstance();
87+
Client client = api.getClient("shouldNotCallEvaluationMethodsWhenProviderIsInFatalErrorState");
11988

120-
assertThat(mockProvider.isEvaluationCalled()).isFalse();
89+
assertThrows(FatalError.class, () -> api.setProviderAndWait("shouldNotCallEvaluationMethodsWhenProviderIsInFatalErrorState", provider));
90+
FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("key", true);
12191
assertThat(details.getErrorCode()).isEqualTo(ErrorCode.PROVIDER_FATAL);
12292
}
12393

12494
@Test
12595
@DisplayName("Should not call evaluation methods when the provider has state NOT_READY")
12696
void shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState() {
127-
MockProvider mockProvider = new MockProvider(ProviderState.NOT_READY);
128-
OpenFeatureAPI api = mock(OpenFeatureAPI.class);
129-
OpenFeatureClient client = new OpenFeatureClient(new MockProviderRepository(mockProvider, false), api, "name", "version");
97+
FeatureProvider provider = new TestEventsProvider(5000);
98+
OpenFeatureAPI api = OpenFeatureAPI.getInstance();
99+
api.setProvider("shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState", provider);
100+
Client client = api.getClient("shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState");
130101
FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("key", true);
131102

132-
assertThat(mockProvider.isEvaluationCalled()).isFalse();
133103
assertThat(details.getErrorCode()).isEqualTo(ErrorCode.PROVIDER_NOT_READY);
134104
}
135105

136-
private static class MockProviderRepository implements ProviderAccessor {
137-
private final FeatureProviderStateManager featureProviderStateManager;
138-
139-
@SneakyThrows
140-
public MockProviderRepository(FeatureProvider featureProvider, boolean init) {
141-
this.featureProviderStateManager = new FeatureProviderStateManager(featureProvider);
142-
if (init) {
143-
this.featureProviderStateManager.initialize(null);
144-
}
145-
}
146-
147-
@Override
148-
public FeatureProviderStateManager getProviderStateManager() {
149-
return featureProviderStateManager;
150-
}
151-
}
152-
153106
private static class MockProvider implements FeatureProvider {
154107
private final AtomicBoolean evaluationCalled = new AtomicBoolean();
155108
private final ProviderState providerState;

‎src/test/java/dev/openfeature/sdk/testutils/TestEventsProvider.java

+12
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package dev.openfeature.sdk.testutils;
22

33
import dev.openfeature.sdk.*;
4+
import dev.openfeature.sdk.exceptions.FatalError;
45
import dev.openfeature.sdk.exceptions.GeneralError;
56
import lombok.SneakyThrows;
67

@@ -13,6 +14,7 @@ public class TestEventsProvider extends EventProvider {
1314
private int initTimeoutMs = 0;
1415
private String name = "test";
1516
private Metadata metadata = () -> name;
17+
private boolean isFatalInitError = false;
1618

1719
public TestEventsProvider() {
1820
}
@@ -27,6 +29,13 @@ public TestEventsProvider(int initTimeoutMs, boolean initError, String initError
2729
this.initErrorMessage = initErrorMessage;
2830
}
2931

32+
public TestEventsProvider(int initTimeoutMs, boolean initError, String initErrorMessage, boolean fatal) {
33+
this.initTimeoutMs = initTimeoutMs;
34+
this.initError = initError;
35+
this.initErrorMessage = initErrorMessage;
36+
this.isFatalInitError = fatal;
37+
}
38+
3039
@SneakyThrows
3140
public static TestEventsProvider newInitializedTestEventsProvider() {
3241
TestEventsProvider provider = new TestEventsProvider();
@@ -52,6 +61,9 @@ public void initialize(EvaluationContext evaluationContext) throws Exception {
5261
// wait half the TIMEOUT, otherwise some init/errors can be fired before we add handlers
5362
Thread.sleep(initTimeoutMs);
5463
if (this.initError) {
64+
if (this.isFatalInitError) {
65+
throw new FatalError(initErrorMessage);
66+
}
5567
throw new GeneralError(initErrorMessage);
5668
}
5769
}

0 commit comments

Comments
 (0)