-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: make provider interface "stateless"; SDK maintains provider state #1096
Merged
toddbaert
merged 31 commits into
open-feature:main
from
chrfwow:make-provider-stateless
Sep 23, 2024
Merged
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
f2e9097
Make provider interface "stateless", SDK maintains provider state
chrfwow d2b15fe
Add tests and fix bug
chrfwow 16f57e8
Merge branch 'main' into make-provider-stateless
chrfwow b993ee2
Fix checkstyle errors
chrfwow 625a404
add test
chrfwow 98c620f
add test
chrfwow d0a2027
implement feedback from codereview
chrfwow 11a0ccb
add wrapper around EventProvider, to manage and hide the provider state
chrfwow 56e66a5
ignore sonar rule
chrfwow 7245963
fixup: flakey tests
toddbaert b90f393
use lombok delegate, add tests and fix codestyle
chrfwow 2a2184b
Merge remote-tracking branch 'origin/make-provider-stateless' into ma…
chrfwow 7d89d52
rename wrapper class, add public facing methods
chrfwow e3c1d30
reduce visibility of emit methods, add error code to ProviderEventDet…
chrfwow 4bc6f66
return provider delegate
chrfwow 8bd726d
fix checkstyle errors
chrfwow 37326d9
make FeatureProviderStateManager a true wrapper, without implementing…
chrfwow b0d66a5
make FeatureProviderStateManager a true wrapper, without implementing…
chrfwow 6bdd7ac
Merge branch 'main' into make-provider-stateless
toddbaert 124be36
fixup: pmd fix and remove equals
toddbaert 8ca0d2d
fixup: revert am change on emit
toddbaert 53dae87
minor refactorings, update readme
chrfwow e4ea124
Merge remote-tracking branch 'origin/make-provider-stateless' into ma…
chrfwow 28130ba
Merge branch 'main' into make-provider-stateless
chrfwow 975d3ad
update readme
chrfwow 01538c3
Merge branch 'main' into make-provider-stateless
chrfwow ca8fc16
remove unused delegate, update comments
chrfwow cd56f25
Merge remote-tracking branch 'origin/make-provider-stateless' into ma…
chrfwow 45e9ba2
fixup: feedback from guido
toddbaert 5816732
fixup: flaky test and spacing
toddbaert 6d199d9
Merge branch 'main' into make-provider-stateless
toddbaert File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,16 @@ | ||
package dev.openfeature.sdk; | ||
|
||
import dev.openfeature.sdk.fixtures.HookFixtures; | ||
import lombok.SneakyThrows; | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertThrows; | ||
import static org.mockito.ArgumentMatchers.any; | ||
import static org.mockito.ArgumentMatchers.anyString; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.never; | ||
|
||
import java.util.HashMap; | ||
import java.util.concurrent.atomic.AtomicBoolean; | ||
|
||
import org.junit.jupiter.api.AfterEach; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.DisplayName; | ||
|
@@ -10,14 +19,9 @@ | |
import org.simplify4u.slf4jmock.LoggerMock; | ||
import org.slf4j.Logger; | ||
|
||
import java.util.Arrays; | ||
import java.util.HashMap; | ||
import java.util.concurrent.atomic.AtomicBoolean; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.mockito.ArgumentMatchers.*; | ||
import static org.mockito.Mockito.*; | ||
import dev.openfeature.sdk.exceptions.FatalError; | ||
import dev.openfeature.sdk.fixtures.HookFixtures; | ||
import dev.openfeature.sdk.testutils.TestEventsProvider; | ||
|
||
class OpenFeatureClientTest implements HookFixtures { | ||
|
||
|
@@ -37,14 +41,10 @@ void reset_logs() { | |
@Test | ||
@DisplayName("should not throw exception if hook has different type argument than hookContext") | ||
void shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext() { | ||
DoSomethingProvider provider = new DoSomethingProvider(); | ||
OpenFeatureAPI api = mock(OpenFeatureAPI.class); | ||
when(api.getProvider(any())).thenReturn(provider); | ||
when(api.getHooks()).thenReturn(Arrays.asList(mockBooleanHook(), mockStringHook())); | ||
|
||
MockProviderRepository mockProviderRepository = new MockProviderRepository(provider, true); | ||
OpenFeatureClient client = new OpenFeatureClient(mockProviderRepository, api, "name", "version"); | ||
|
||
OpenFeatureAPI api = OpenFeatureAPI.getInstance(); | ||
api.setProvider("shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext", new DoSomethingProvider()); | ||
Client client = api.getClient("shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext"); | ||
client.addHooks(mockBooleanHook(), mockStringHook()); | ||
FlagEvaluationDetails<Boolean> actual = client.getBooleanDetails("feature key", Boolean.FALSE); | ||
|
||
assertThat(actual.getValue()).isTrue(); | ||
|
@@ -56,36 +56,11 @@ void shouldNotThrowExceptionIfHookHasDifferentTypeArgumentThanHookContext() { | |
Mockito.verify(logger, never()).error(anyString(), any(), any()); | ||
} | ||
|
||
@Test | ||
void mergeContextTest() { | ||
String flag = "feature key"; | ||
boolean defaultValue = false; | ||
String targetingKey = "targeting key"; | ||
EvaluationContext ctx = new ImmutableContext(targetingKey, new HashMap<>()); | ||
OpenFeatureAPI api = mock(OpenFeatureAPI.class); | ||
FeatureProvider mockProvider = mock(FeatureProvider.class); | ||
// this makes it so that true is returned only if the targeting key set at the client level is honored | ||
when(mockProvider.getBooleanEvaluation( | ||
eq(flag), eq(defaultValue), argThat( | ||
context -> context.getTargetingKey().equals(targetingKey)))).thenReturn(ProviderEvaluation.<Boolean>builder() | ||
.value(true).build()); | ||
when(api.getProvider()).thenReturn(mockProvider); | ||
when(api.getProvider(any())).thenReturn(mockProvider); | ||
|
||
MockProviderRepository mockProviderRepository = new MockProviderRepository(mockProvider, true); | ||
OpenFeatureClient client = new OpenFeatureClient(mockProviderRepository, api, "name", "version"); | ||
client.setEvaluationContext(ctx); | ||
|
||
FlagEvaluationDetails<Boolean> result = client.getBooleanDetails(flag, defaultValue); | ||
|
||
assertThat(result.getValue()).isTrue(); | ||
} | ||
|
||
beeme1mr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
@Test | ||
@DisplayName("addHooks should allow chaining by returning the same client instance") | ||
void addHooksShouldAllowChaining() { | ||
OpenFeatureAPI api = mock(OpenFeatureAPI.class); | ||
OpenFeatureClient client = new OpenFeatureClient(() -> null, api, "name", "version"); | ||
OpenFeatureClient client = new OpenFeatureClient(api, "name", "version"); | ||
Hook<?> hook1 = Mockito.mock(Hook.class); | ||
Hook<?> hook2 = Mockito.mock(Hook.class); | ||
|
||
|
@@ -97,7 +72,7 @@ void addHooksShouldAllowChaining() { | |
@DisplayName("setEvaluationContext should allow chaining by returning the same client instance") | ||
void setEvaluationContextShouldAllowChaining() { | ||
OpenFeatureAPI api = mock(OpenFeatureAPI.class); | ||
OpenFeatureClient client = new OpenFeatureClient(() -> null, api, "name", "version"); | ||
OpenFeatureClient client = new OpenFeatureClient(api, "name", "version"); | ||
EvaluationContext ctx = new ImmutableContext("targeting key", new HashMap<>()); | ||
|
||
OpenFeatureClient result = client.setEvaluationContext(ctx); | ||
|
@@ -107,49 +82,27 @@ void setEvaluationContextShouldAllowChaining() { | |
@Test | ||
@DisplayName("Should not call evaluation methods when the provider has state FATAL") | ||
void shouldNotCallEvaluationMethodsWhenProviderIsInFatalErrorState() { | ||
MockProvider mockProvider = new MockProvider(ProviderState.FATAL); | ||
OpenFeatureAPI api = mock(OpenFeatureAPI.class); | ||
MockProviderRepository mockProviderRepository = new MockProviderRepository(mockProvider, true); | ||
OpenFeatureClient client = new OpenFeatureClient(mockProviderRepository, api, "name", "version"); | ||
mockProviderRepository.featureProviderStateManager.onEmit( | ||
ProviderEvent.PROVIDER_ERROR, | ||
ProviderEventDetails.builder().errorCode(ErrorCode.PROVIDER_FATAL).build() | ||
); | ||
FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("key", true); | ||
FeatureProvider provider = new TestEventsProvider(100, true, "fake fatal", true); | ||
OpenFeatureAPI api = OpenFeatureAPI.getInstance(); | ||
Client client = api.getClient("shouldNotCallEvaluationMethodsWhenProviderIsInFatalErrorState"); | ||
|
||
assertThat(mockProvider.isEvaluationCalled()).isFalse(); | ||
assertThrows(FatalError.class, () -> api.setProviderAndWait("shouldNotCallEvaluationMethodsWhenProviderIsInFatalErrorState", provider)); | ||
FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("key", true); | ||
assertThat(details.getErrorCode()).isEqualTo(ErrorCode.PROVIDER_FATAL); | ||
} | ||
|
||
@Test | ||
@DisplayName("Should not call evaluation methods when the provider has state NOT_READY") | ||
void shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState() { | ||
MockProvider mockProvider = new MockProvider(ProviderState.NOT_READY); | ||
OpenFeatureAPI api = mock(OpenFeatureAPI.class); | ||
OpenFeatureClient client = new OpenFeatureClient(new MockProviderRepository(mockProvider, false), api, "name", "version"); | ||
FeatureProvider provider = new TestEventsProvider(5000); | ||
OpenFeatureAPI api = OpenFeatureAPI.getInstance(); | ||
api.setProvider("shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState", provider); | ||
Client client = api.getClient("shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState"); | ||
FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("key", true); | ||
|
||
assertThat(mockProvider.isEvaluationCalled()).isFalse(); | ||
assertThat(details.getErrorCode()).isEqualTo(ErrorCode.PROVIDER_NOT_READY); | ||
} | ||
|
||
private static class MockProviderRepository implements ProviderAccessor { | ||
private final FeatureProviderStateManager featureProviderStateManager; | ||
|
||
@SneakyThrows | ||
public MockProviderRepository(FeatureProvider featureProvider, boolean init) { | ||
this.featureProviderStateManager = new FeatureProviderStateManager(featureProvider); | ||
if (init) { | ||
this.featureProviderStateManager.initialize(null); | ||
} | ||
} | ||
|
||
@Override | ||
public FeatureProviderStateManager getProviderStateManager() { | ||
return featureProviderStateManager; | ||
} | ||
} | ||
|
||
private static class MockProvider implements FeatureProvider { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might be able to use the |
||
private final AtomicBoolean evaluationCalled = new AtomicBoolean(); | ||
private final ProviderState providerState; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My change:
This test was flaky, since after your changes, it was possible that the provider was initialized before line 105 ran. This adds an artificial delay to init to make sure.
Notice that I'm NOT using
setProviderAndWait
here because we explicitly DONT want the provider to be ready for this test.