Skip to content
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
merged 31 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
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
Sep 9, 2024
d2b15fe
Add tests and fix bug
Sep 9, 2024
16f57e8
Merge branch 'main' into make-provider-stateless
chrfwow Sep 10, 2024
b993ee2
Fix checkstyle errors
Sep 10, 2024
625a404
add test
chrfwow Sep 10, 2024
98c620f
add test
Sep 10, 2024
d0a2027
implement feedback from codereview
chrfwow Sep 11, 2024
11a0ccb
add wrapper around EventProvider, to manage and hide the provider state
Sep 12, 2024
56e66a5
ignore sonar rule
Sep 12, 2024
7245963
fixup: flakey tests
toddbaert Sep 12, 2024
b90f393
use lombok delegate, add tests and fix codestyle
Sep 13, 2024
2a2184b
Merge remote-tracking branch 'origin/make-provider-stateless' into ma…
Sep 13, 2024
7d89d52
rename wrapper class, add public facing methods
Sep 13, 2024
e3c1d30
reduce visibility of emit methods, add error code to ProviderEventDet…
chrfwow Sep 16, 2024
4bc6f66
return provider delegate
chrfwow Sep 16, 2024
8bd726d
fix checkstyle errors
Sep 16, 2024
37326d9
make FeatureProviderStateManager a true wrapper, without implementing…
chrfwow Sep 18, 2024
b0d66a5
make FeatureProviderStateManager a true wrapper, without implementing…
Sep 18, 2024
6bdd7ac
Merge branch 'main' into make-provider-stateless
toddbaert Sep 18, 2024
124be36
fixup: pmd fix and remove equals
toddbaert Sep 18, 2024
8ca0d2d
fixup: revert am change on emit
toddbaert Sep 18, 2024
53dae87
minor refactorings, update readme
chrfwow Sep 19, 2024
e4ea124
Merge remote-tracking branch 'origin/make-provider-stateless' into ma…
Sep 19, 2024
28130ba
Merge branch 'main' into make-provider-stateless
chrfwow Sep 19, 2024
975d3ad
update readme
Sep 19, 2024
01538c3
Merge branch 'main' into make-provider-stateless
chrfwow Sep 20, 2024
ca8fc16
remove unused delegate, update comments
chrfwow Sep 20, 2024
cd56f25
Merge remote-tracking branch 'origin/make-provider-stateless' into ma…
Sep 20, 2024
45e9ba2
fixup: feedback from guido
toddbaert Sep 23, 2024
5816732
fixup: flaky test and spacing
toddbaert Sep 23, 2024
6d199d9
Merge branch 'main' into make-provider-stateless
toddbaert Sep 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/main/java/dev/openfeature/sdk/Client.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,10 @@ public interface Client extends Features, EventBus<Client> {
* @return A list of {@link Hook}s.
*/
List<Hook> getHooks();

/**
* Returns the current state of the associated provider
* @return the provider state
*/
ProviderState getProviderState();
}
2 changes: 1 addition & 1 deletion src/main/java/dev/openfeature/sdk/ErrorCode.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@

@SuppressWarnings("checkstyle:MissingJavadocType")
public enum ErrorCode {
PROVIDER_NOT_READY, FLAG_NOT_FOUND, PARSE_ERROR, TYPE_MISMATCH, TARGETING_KEY_MISSING, INVALID_CONTEXT, GENERAL
PROVIDER_NOT_READY, FLAG_NOT_FOUND, PARSE_ERROR, TYPE_MISMATCH, TARGETING_KEY_MISSING, INVALID_CONTEXT, GENERAL, PROVIDER_FATAL
}
60 changes: 52 additions & 8 deletions src/main/java/dev/openfeature/sdk/EventProvider.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package dev.openfeature.sdk;

import dev.openfeature.sdk.exceptions.GeneralError;
import dev.openfeature.sdk.exceptions.OpenFeatureError;
import dev.openfeature.sdk.internal.TriConsumer;

/**
Expand All @@ -16,21 +18,56 @@
*/
public abstract class EventProvider implements FeatureProvider {

private ProviderState providerState = ProviderState.NOT_READY;

/**
* {@inheritDoc}
*/
@Override
public abstract ProviderState getState();
public final ProviderState getState() {
return providerState;
}

@Override
public final void initialize(EvaluationContext evaluationContext) throws Exception {
try {
doInitialization(evaluationContext);
providerState = ProviderState.READY;
} catch (OpenFeatureError openFeatureError) {
if (ErrorCode.PROVIDER_FATAL.equals(openFeatureError.getErrorCode())) {
providerState = ProviderState.FATAL;
} else {
providerState = ProviderState.ERROR;
}
throw openFeatureError;
} catch (Exception e) {
providerState = ProviderState.ERROR;
throw new GeneralError(e);
}
}

protected void doInitialization(EvaluationContext evaluationContext) throws Exception {
// Intentionally left blank, to be implemented by inheritors
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is logically correct, but this won't work retroactively with existing implementations (many of which can be found here, though there's others maintained by vendors, etc).

We need to do this in a non-breaking way, without breaking any existing contracts or behvaiors. In C#, I accomplished this by creating a new package-private ProviderState field on the provider which the SDK updates after it has successfully (or unsuccessfully) run the initialize method, and basically ignoring the previous provider state field.

This allows existing providers to work without breaking changes.

Copy link
Member

@liran2000 liran2000 Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toddbaert I wonder if there can be some additional automated step to see if the sdk-contrib repository providers are not breaking, possibly as warning / non-mandatory. There is the InMemoryProvider on this repository which is a good sanity, maybe checking the other providers as well can help. Or is too much? What do you think ?


@Override
public final void shutdown() {
providerState = ProviderState.NOT_READY;
doShutdown();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this split is needed ?
without it, provider can implement shutdown() same as today and calling super.shutdown() in it.
providers which were implemented this way, can continue to work as is.
similar with initialize().

}

protected void doShutdown() {
// Intentionally left blank, to be implemented by inheritors
}

private TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit = null;

/**
* "Attach" this EventProvider to an SDK, which allows events to propagate from this provider.
* No-op if the same onEmit is already attached.
* No-op if the same onEmit is already attached.
*
* @param onEmit the function to run when a provider emits events.
* @throws IllegalStateException if attempted to bind a new emitter for already bound provider
*
*/
void attach(TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit) {
if (this.onEmit != null && this.onEmit != onEmit) {
Expand All @@ -50,11 +87,18 @@ void detach() {

/**
* Emit the specified {@link ProviderEvent}.
*
*
* @param event The event type
* @param details The details of the event
*/
public void emit(ProviderEvent event, ProviderEventDetails details) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR, but I'm wondering if public visibility of emit methods is too generous. Is there a use-case where anyone outside the provider itself would want to emit a provider state change directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good point, and we can probably reduce this visibility without considering this a breaking change since events are still labeled "experimental" so that would break our stability guarantee.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrfwow did this but I had to revert it. We had multiple providers in the contribs who used an encapsulation strategy and used this publicly instead of in a subclass.

if (ProviderEvent.PROVIDER_ERROR.equals(event)) {
providerState = ProviderState.ERROR;
} else if (ProviderEvent.PROVIDER_STALE.equals(event)) {
providerState = ProviderState.STALE;
} else if (ProviderEvent.PROVIDER_READY.equals(event)) {
providerState = ProviderState.READY;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a specific test for this? It relates to specification requirement 5.3.5, so you can use our @Specification annotation for tracking compliance.

if (this.onEmit != null) {
this.onEmit.accept(this, event, details);
}
Expand All @@ -63,7 +107,7 @@ public void emit(ProviderEvent event, ProviderEventDetails details) {
/**
* Emit a {@link ProviderEvent#PROVIDER_READY} event.
* Shorthand for {@link #emit(ProviderEvent, ProviderEventDetails)}
*
*
* @param details The details of the event
*/
public void emitProviderReady(ProviderEventDetails details) {
Expand All @@ -74,7 +118,7 @@ public void emitProviderReady(ProviderEventDetails details) {
* Emit a
* {@link ProviderEvent#PROVIDER_CONFIGURATION_CHANGED}
* event. Shorthand for {@link #emit(ProviderEvent, ProviderEventDetails)}
*
*
* @param details The details of the event
*/
public void emitProviderConfigurationChanged(ProviderEventDetails details) {
Expand All @@ -84,7 +128,7 @@ public void emitProviderConfigurationChanged(ProviderEventDetails details) {
/**
* Emit a {@link ProviderEvent#PROVIDER_STALE} event.
* Shorthand for {@link #emit(ProviderEvent, ProviderEventDetails)}
*
*
* @param details The details of the event
*/
public void emitProviderStale(ProviderEventDetails details) {
Expand All @@ -94,7 +138,7 @@ public void emitProviderStale(ProviderEventDetails details) {
/**
* Emit a {@link ProviderEvent#PROVIDER_ERROR} event.
* Shorthand for {@link #emit(ProviderEvent, ProviderEventDetails)}
*
*
* @param details The details of the event
*/
public void emitProviderError(ProviderEventDetails details) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there also be a method like emitFatalProviderError?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would take the approach of just checking if the error is a Fatal error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I do that? The ProviderEventDetails parameter does not have a field containing a potential error code. If I can get it from getEventMetadata(), what key is the error code associated with?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think that's another thing we need to add. I might have missed it in the issue, but it's in the spec: https://openfeature.dev/specification/types#event-details

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it queried like this?

String errorCode = details.getEventMetadata().getString("error code");
if("FATAL".equals(errorCode)){
    ...
}

I can't see how this should work from the documentation

Copy link
Member

@toddbaert toddbaert Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm saying the event details should have a first class member representing the error code (not in the event data), if it's an error event.

Expand Down
30 changes: 15 additions & 15 deletions src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,7 @@
import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock;
import lombok.extern.slf4j.Slf4j;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.*;
import java.util.function.Consumer;

/**
Expand Down Expand Up @@ -86,7 +82,7 @@ public Client getClient() {
* Multiple clients can be used to segment feature flag configuration.
* If there is already a provider bound to this domain, this provider will be used.
* Otherwise, the default provider is used until a provider is assigned to that domain.
*
*
* @param domain an identifier which logically binds clients with providers
* @return a new client instance
*/
Expand All @@ -100,15 +96,18 @@ public Client getClient(String domain) {
* Multiple clients can be used to segment feature flag configuration.
* If there is already a provider bound to this domain, this provider will be used.
* Otherwise, the default provider is used until a provider is assigned to that domain.
*
* @param domain a identifier which logically binds clients with providers
*
* @param domain a identifier which logically binds clients with providers
* @param version a version identifier
* @return a new client instance
*/
public Client getClient(String domain, String version) {
return new OpenFeatureClient(this,
return new OpenFeatureClient(
() -> providerRepository.getProvider(domain),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside the ProviderRespository, you will have to modify this private method to call the initialize function on the provider if it's not yet initialized or in the process of being initialized, then update a new package-private status member indicating readiness.

this,
domain,
version);
version
);
}

/**
Expand Down Expand Up @@ -193,8 +192,8 @@ public void setProvider(FeatureProvider provider) {
/**
* Add a provider for a domain.
*
* @param domain The domain to bind the provider to.
* @param provider The provider to set.
* @param domain The domain to bind the provider to.
* @param provider The provider to set.
*/
public void setProvider(String domain, FeatureProvider provider) {
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
Expand Down Expand Up @@ -226,8 +225,8 @@ public void setProviderAndWait(FeatureProvider provider) throws OpenFeatureError
/**
* Add a provider for a domain and wait for initialization to finish.
*
* @param domain The domain to bind the provider to.
* @param provider The provider to set.
* @param domain The domain to bind the provider to.
* @param provider The provider to set.
*/
public void setProviderAndWait(String domain, FeatureProvider provider) throws OpenFeatureError {
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
Expand Down Expand Up @@ -300,6 +299,7 @@ public void addHooks(Hook... hooks) {

/**
* Fetch the hooks associated to this client.
*
* @return A list of {@link Hook}s.
*/
public List<Hook> getHooks() {
Expand Down Expand Up @@ -404,7 +404,7 @@ void addHandler(String domain, ProviderEvent event, Consumer<EventDetails> handl

/**
* Runs the handlers associated with a particular provider.
*
*
* @param provider the provider from where this event originated
* @param event the event type
* @param details the event details
Expand Down
Loading
Loading