Skip to content
Merged
Show file tree
Hide file tree
Changes from 28 commits
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 Sep 9, 2024
d2b15fe
Add tests and fix bug
chrfwow Sep 9, 2024
16f57e8
Merge branch 'main' into make-provider-stateless
chrfwow Sep 10, 2024
b993ee2
Fix checkstyle errors
chrfwow Sep 10, 2024
625a404
add test
chrfwow Sep 10, 2024
98c620f
add test
chrfwow Sep 10, 2024
d0a2027
implement feedback from codereview
chrfwow Sep 11, 2024
11a0ccb
add wrapper around EventProvider, to manage and hide the provider state
chrfwow Sep 12, 2024
56e66a5
ignore sonar rule
chrfwow Sep 12, 2024
7245963
fixup: flakey tests
toddbaert Sep 12, 2024
b90f393
use lombok delegate, add tests and fix codestyle
chrfwow Sep 13, 2024
2a2184b
Merge remote-tracking branch 'origin/make-provider-stateless' into ma…
chrfwow Sep 13, 2024
7d89d52
rename wrapper class, add public facing methods
chrfwow 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
chrfwow Sep 16, 2024
37326d9
make FeatureProviderStateManager a true wrapper, without implementing…
chrfwow Sep 18, 2024
b0d66a5
make FeatureProviderStateManager a true wrapper, without implementing…
chrfwow 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…
chrfwow Sep 19, 2024
28130ba
Merge branch 'main' into make-provider-stateless
chrfwow Sep 19, 2024
975d3ad
update readme
chrfwow 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…
chrfwow 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
17 changes: 7 additions & 10 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -317,11 +317,6 @@ public class MyProvider implements FeatureProvider {
return () -> "My Provider";
}

@Override
public ProviderState getState() {
// optionally indicate your provider's state (assumed to be READY if not implemented)
}

@Override
public void initialize(EvaluationContext evaluationContext) throws Exception {
// start up your provider
Expand Down Expand Up @@ -368,11 +363,6 @@ class MyEventProvider extends EventProvider {
return () -> "My Event Provider";
}

@Override
public ProviderState getState() {
// indicate your provider's state (required for EventProviders)
}

@Override
public void initialize(EvaluationContext evaluationContext) throws Exception {
// emit events when flags are changed in a hypothetical REST API
Expand All @@ -391,6 +381,13 @@ class MyEventProvider extends EventProvider {
}
```

Providers no longer need to manage their own state, this is done by the SDK itself. If desired, the state of a provider
can be queried through the client that uses the provider.

```java
OpenFeatureAPI.getInstance().getClient().getProviderState();
```

> Built a new provider? [Let us know](https://github.com/open-feature/openfeature.dev/issues/new?assignees=&labels=provider&projects=&template=document-provider.yaml&title=%5BProvider%5D%3A+) so we can add it to the docs!

### Develop a hook
Expand Down
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();
}
3 changes: 2 additions & 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,6 @@

@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
}
26 changes: 14 additions & 12 deletions src/main/java/dev/openfeature/sdk/EventProvider.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import dev.openfeature.sdk.internal.TriConsumer;


/**
* Abstract EventProvider. Providers must extend this class to support events.
* Emit events with {@link #emit(ProviderEvent, ProviderEventDetails)}. Please
Expand All @@ -15,22 +16,20 @@
* @see FeatureProvider
*/
public abstract class EventProvider implements FeatureProvider {
private EventProviderListener eventProviderListener;

/**
* {@inheritDoc}
*/
@Override
public abstract ProviderState getState();
void setEventProviderListener(EventProviderListener eventProviderListener) {
this.eventProviderListener = eventProviderListener;
}

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 +49,14 @@ 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 (eventProviderListener != null) {
eventProviderListener.onEmit(event, details);
}
if (this.onEmit != null) {
this.onEmit.accept(this, event, details);
}
Expand All @@ -63,7 +65,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 +76,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 +86,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 +96,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
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package dev.openfeature.sdk;

@FunctionalInterface
interface EventProviderListener {
void onEmit(ProviderEvent event, ProviderEventDetails details);
}
6 changes: 4 additions & 2 deletions src/main/java/dev/openfeature/sdk/FeatureProvider.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,13 @@ default void shutdown() {
* If the provider needs to be initialized, it should return {@link ProviderState#NOT_READY}.
* If the provider is in an error state, it should return {@link ProviderState#ERROR}.
* If the provider is functioning normally, it should return {@link ProviderState#READY}.
*
*
* <p><i>Providers which do not implement this method are assumed to be ready immediately.</i></p>
*
*
* @return ProviderState
* @deprecated The state is handled by the SDK internally. Query the state from the {@link Client} instead.
*/
@Deprecated
default ProviderState getState() {
return ProviderState.READY;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package dev.openfeature.sdk;

import dev.openfeature.sdk.exceptions.OpenFeatureError;
import lombok.Getter;

import java.util.concurrent.atomic.AtomicBoolean;

class FeatureProviderStateManager implements EventProviderListener {
private final FeatureProvider delegate;
private final AtomicBoolean isInitialized = new AtomicBoolean();
@Getter
private ProviderState state = ProviderState.NOT_READY;

public FeatureProviderStateManager(FeatureProvider delegate) {
this.delegate = delegate;
if (delegate instanceof EventProvider) {
((EventProvider) delegate).setEventProviderListener(this);
}
}

public void initialize(EvaluationContext evaluationContext) throws Exception {
if (isInitialized.getAndSet(true)) {
return;
}
try {
delegate.initialize(evaluationContext);
state = ProviderState.READY;
} catch (OpenFeatureError openFeatureError) {
if (ErrorCode.PROVIDER_FATAL.equals(openFeatureError.getErrorCode())) {
state = ProviderState.FATAL;
} else {
state = ProviderState.ERROR;
}
isInitialized.set(false);
throw openFeatureError;
} catch (Exception e) {
state = ProviderState.ERROR;
isInitialized.set(false);
throw e;
}
}

public void shutdown() {
delegate.shutdown();
state = ProviderState.NOT_READY;
isInitialized.set(false);
}

@Override
public void onEmit(ProviderEvent event, ProviderEventDetails details) {
if (ProviderEvent.PROVIDER_ERROR.equals(event)) {
if (details != null && details.getErrorCode() == ErrorCode.PROVIDER_FATAL) {
state = ProviderState.FATAL;
} else {
state = ProviderState.ERROR;
}
} else if (ProviderEvent.PROVIDER_STALE.equals(event)) {
state = ProviderState.STALE;
} else if (ProviderEvent.PROVIDER_READY.equals(event)) {
state = ProviderState.READY;
}
}

FeatureProvider getProvider() {
return delegate;
}

public boolean hasSameProvider(FeatureProvider featureProvider) {
return this.delegate.equals(featureProvider);
}
}
2 changes: 1 addition & 1 deletion src/main/java/dev/openfeature/sdk/NoOpProvider.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public ProviderEvaluation<Double> getDoubleEvaluation(String key, Double default

@Override
public ProviderEvaluation<Value> getObjectEvaluation(String key, Value defaultValue,
EvaluationContext invocationContext) {
EvaluationContext invocationContext) {
return ProviderEvaluation.<Value>builder()
.value(defaultValue)
.variant(PASSED_IN_DEFAULT)
Expand Down
60 changes: 41 additions & 19 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 @@ -69,7 +65,7 @@ public Metadata getProviderMetadata(String domain) {
}

/**
* A factory function for creating new, OpenFeature clients.
* A factory function for creating new, OpenFeature client.
* Clients can contain their own state (e.g. logger, hook, context).
* Multiple clients can be used to segment feature flag configuration.
* All un-named or unbound clients use the default provider.
Expand All @@ -81,12 +77,12 @@ public Client getClient() {
}

/**
* A factory function for creating new domainless OpenFeature clients.
* A factory function for creating new domainless OpenFeature client.
* Clients can contain their own state (e.g. logger, hook, context).
* 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 @@ -95,20 +91,23 @@ public Client getClient(String domain) {
}

/**
* A factory function for creating new domainless OpenFeature clients.
* A factory function for creating new domainless OpenFeature client.
* Clients can contain their own state (e.g. logger, hook, context).
* 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.getFeatureProviderStateManager(domain),
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 @@ -286,6 +285,28 @@ public FeatureProvider getProvider(String domain) {
return providerRepository.getProvider(domain);
}

/**
* Return the state of the default provider.
*/
public ProviderState getProviderState() {
return providerRepository.getProviderState();
}

/**
* Get the state of the provider for a domain. If no provider with the domain is found, returns the state of the
* default provider.
*
* @param domain The domain to look for.
* @return A named {@link FeatureProvider}
*/
public ProviderState getProviderState(String domain) {
Copy link
Member

Choose a reason for hiding this comment

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

If we make provider state accessible here, I would also add an overload for public ProviderState getProviderState(FeatureProvider provider).

Although, I like the existing concept of getState() beeing a member of the Provider itself better, because it's clearly belonging to the provider.

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.

Copy link
Member

Choose a reason for hiding this comment

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

@guidobrei sorry, I put this comment in the wrong place. I meant for it to be about making the emit protected, which we had to revert (https://github.com/open-feature/java-sdk/pull/1096/files/cd56f25cbfb918117ec53a69ec1d55a87ca6422b#r1758249819)

I think you're right about this one. I don't think these need to be public.

return providerRepository.getProviderState(domain);
}

public ProviderState getProviderState(FeatureProvider provider) {
Copy link
Member

Choose a reason for hiding this comment

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

I know I've requested this, but maybe it's better to not expose this on the public API surface unless someone requests it. We could also get rid of the additional code in the ProviderRepository that handles this request again.

Sorry for being too greedy 🙈

Copy link
Member

Choose a reason for hiding this comment

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

This overload would return a null state if the provider is not registered, which is what I would expect (or an exception?), but it's still different to the behavior of other getProviderState() overloads.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I guess the only reason this was done was for this assertion which we can avoid. I agree we should remove these methods.

Copy link
Member

Choose a reason for hiding this comment

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

Done: 45e9ba2

return providerRepository.getProviderState(provider);
}

/**
* Adds hooks for globally, used for all evaluations.
* Hooks are run in the order they're added in the before stage. They are run in reverse order for all other stages.
Expand All @@ -300,6 +321,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 @@ -394,7 +416,7 @@ void removeHandler(String domain, ProviderEvent event, Consumer<EventDetails> ha
void addHandler(String domain, ProviderEvent event, Consumer<EventDetails> handler) {
try (AutoCloseableLock __ = lock.writeLockAutoCloseable()) {
// if the provider is in the state associated with event, run immediately
if (Optional.ofNullable(this.providerRepository.getProvider(domain).getState())
if (Optional.ofNullable(this.providerRepository.getProviderState(domain))
.orElse(ProviderState.READY).matchesEvent(event)) {
eventSupport.runHandler(handler, EventDetails.builder().domain(domain).build());
}
Expand All @@ -404,7 +426,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