From 98eb2c2d8fd4c144acdeb6bc51052b24742dbfdb Mon Sep 17 00:00:00 2001 From: Norris Date: Fri, 6 Mar 2026 11:00:34 +0100 Subject: [PATCH 1/8] Cursor: Apply local changes for cloud agent Signed-off-by: Jonathan Norris --- mvnw | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 mvnw diff --git a/mvnw b/mvnw old mode 100644 new mode 100755 From 85bf6bf0a7984981d268652e1bdbfed0b1ec01ca Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 6 Mar 2026 10:55:00 +0000 Subject: [PATCH 2/8] fix multiprovider parity gaps from issue 1882 Co-authored-by: jonathan Signed-off-by: Jonathan Norris --- .../dev/openfeature/sdk/EventProvider.java | 39 +- .../sdk/multiprovider/ComparisonStrategy.java | 164 ++++++ .../sdk/multiprovider/MultiProvider.java | 540 +++++++++++++++++- .../multiprovider/ComparisonStrategyTest.java | 103 ++++ .../MultiProviderEventsAndTrackingTest.java | 145 +++++ .../multiprovider/MultiProviderHooksTest.java | 157 +++++ .../sdk/multiprovider/MultiProviderTest.java | 7 +- 7 files changed, 1136 insertions(+), 19 deletions(-) create mode 100644 src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java create mode 100644 src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java create mode 100644 src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java create mode 100644 src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java diff --git a/src/main/java/dev/openfeature/sdk/EventProvider.java b/src/main/java/dev/openfeature/sdk/EventProvider.java index c126c1451..a02191606 100644 --- a/src/main/java/dev/openfeature/sdk/EventProvider.java +++ b/src/main/java/dev/openfeature/sdk/EventProvider.java @@ -2,9 +2,12 @@ import dev.openfeature.sdk.internal.ConfigurableThreadFactory; import dev.openfeature.sdk.internal.TriConsumer; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; +import java.util.function.BiConsumer; import lombok.extern.slf4j.Slf4j; /** @@ -22,6 +25,7 @@ @Slf4j public abstract class EventProvider implements FeatureProvider { private EventProviderListener eventProviderListener; + private final List> eventObservers = new CopyOnWriteArrayList<>(); private final ExecutorService emitterExecutor = Executors.newCachedThreadPool(new ConfigurableThreadFactory("openfeature-event-emitter-thread", true)); @@ -54,6 +58,31 @@ void detach() { this.onEmit = null; } + /** + * Add a provider event observer. + * + *

Observers are invoked whenever this provider emits an event and are intended for advanced + * provider composition scenarios. + * + * @param observer observer callback + */ + public void addEventObserver(BiConsumer observer) { + if (observer != null) { + eventObservers.add(observer); + } + } + + /** + * Remove a previously registered provider event observer. + * + * @param observer observer callback + */ + public void removeEventObserver(BiConsumer observer) { + if (observer != null) { + eventObservers.remove(observer); + } + } + /** * Stop the event emitter executor and block until either termination has completed * or timeout period has elapsed. @@ -81,8 +110,9 @@ public void shutdown() { public Awaitable emit(final ProviderEvent event, final ProviderEventDetails details) { final var localEventProviderListener = this.eventProviderListener; final var localOnEmit = this.onEmit; + final var localEventObservers = this.eventObservers; - if (localEventProviderListener == null && localOnEmit == null) { + if (localEventProviderListener == null && localOnEmit == null && localEventObservers.isEmpty()) { return Awaitable.FINISHED; } @@ -98,6 +128,13 @@ public Awaitable emit(final ProviderEvent event, final ProviderEventDetails deta if (localOnEmit != null) { localOnEmit.accept(this, event, details); } + for (BiConsumer observer : localEventObservers) { + try { + observer.accept(event, details); + } catch (Exception e) { + log.error("Exception in provider event observer {}", observer, e); + } + } } finally { awaitable.wakeup(); } diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java b/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java new file mode 100644 index 000000000..d2ba63aca --- /dev/null +++ b/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java @@ -0,0 +1,164 @@ +package dev.openfeature.sdk.multiprovider; + +import dev.openfeature.sdk.ErrorCode; +import dev.openfeature.sdk.EvaluationContext; +import dev.openfeature.sdk.FeatureProvider; +import dev.openfeature.sdk.ProviderEvaluation; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.Callable; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.function.BiConsumer; +import java.util.function.Function; +import lombok.Getter; + +/** + * Comparison strategy. + * + *

Evaluates all providers and compares successful results. + */ +public class ComparisonStrategy implements Strategy { + + @Getter + private final String fallbackProvider; + + private final BiConsumer>> onMismatch; + + /** + * Constructs a comparison strategy with a fallback provider. + * + * @param fallbackProvider provider name to use as fallback when successful providers disagree + */ + public ComparisonStrategy(String fallbackProvider) { + this(fallbackProvider, null); + } + + /** + * Constructs a comparison strategy with fallback provider and mismatch callback. + * + * @param fallbackProvider provider name to use as fallback when successful providers disagree + * @param onMismatch callback invoked with all successful evaluations when they disagree + */ + public ComparisonStrategy( + String fallbackProvider, + BiConsumer>> onMismatch) { + this.fallbackProvider = Objects.requireNonNull(fallbackProvider, "fallbackProvider must not be null"); + this.onMismatch = onMismatch; + } + + @Override + public ProviderEvaluation evaluate( + Map providers, + String key, + T defaultValue, + EvaluationContext ctx, + Function> providerFunction) { + if (providers.isEmpty()) { + return ProviderEvaluation.builder() + .errorCode(ErrorCode.GENERAL) + .errorMessage("No providers configured") + .build(); + } + if (!providers.containsKey(fallbackProvider)) { + throw new IllegalArgumentException("fallbackProvider not found in providers: " + fallbackProvider); + } + + Map> successfulResults = new ConcurrentHashMap<>(providers.size()); + Map providerErrors = new ConcurrentHashMap<>(providers.size()); + ExecutorService executorService = Executors.newFixedThreadPool(providers.size()); + try { + List> tasks = new ArrayList<>(providers.size()); + for (Map.Entry entry : providers.entrySet()) { + String providerName = entry.getKey(); + FeatureProvider provider = entry.getValue(); + tasks.add(() -> { + try { + ProviderEvaluation evaluation = providerFunction.apply(provider); + if (evaluation == null) { + providerErrors.put(providerName, "null evaluation"); + } else if (evaluation.getErrorCode() == null) { + successfulResults.put(providerName, evaluation); + } else { + providerErrors.put( + providerName, + evaluation.getErrorCode() + ": " + String.valueOf(evaluation.getErrorMessage())); + } + } catch (Exception e) { + providerErrors.put(providerName, e.getClass().getSimpleName() + ": " + e.getMessage()); + } + return null; + }); + } + List> futures = executorService.invokeAll(tasks); + for (Future future : futures) { + future.get(); + } + } catch (Exception e) { + return ProviderEvaluation.builder() + .errorCode(ErrorCode.GENERAL) + .errorMessage("Comparison strategy failed: " + e.getMessage()) + .build(); + } finally { + executorService.shutdown(); + } + + if (!providerErrors.isEmpty()) { + return ProviderEvaluation.builder() + .errorCode(ErrorCode.GENERAL) + .errorMessage("Provider errors: " + buildErrorSummary(providerErrors)) + .build(); + } + + ProviderEvaluation fallbackResult = successfulResults.get(fallbackProvider); + if (fallbackResult == null) { + return ProviderEvaluation.builder() + .errorCode(ErrorCode.GENERAL) + .errorMessage("Fallback provider did not return a successful evaluation: " + fallbackProvider) + .build(); + } + + if (allEvaluationsMatch(successfulResults)) { + return fallbackResult; + } + + if (onMismatch != null) { + Map> mismatchPayload = new LinkedHashMap<>(successfulResults); + onMismatch.accept(key, Collections.unmodifiableMap(mismatchPayload)); + } + return fallbackResult; + } + + private String buildErrorSummary(Map providerErrors) { + StringBuilder builder = new StringBuilder(); + boolean first = true; + for (Map.Entry entry : providerErrors.entrySet()) { + if (!first) { + builder.append("; "); + } + first = false; + builder.append(entry.getKey()).append(" -> ").append(entry.getValue()); + } + return builder.toString(); + } + + private boolean allEvaluationsMatch(Map> results) { + ProviderEvaluation baseline = null; + for (ProviderEvaluation evaluation : results.values()) { + if (baseline == null) { + baseline = evaluation; + continue; + } + if (!Objects.equals(baseline.getValue(), evaluation.getValue())) { + return false; + } + } + return true; + } +} diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java index cc6fb8db2..937a10458 100644 --- a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java +++ b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java @@ -1,11 +1,26 @@ package dev.openfeature.sdk.multiprovider; +import dev.openfeature.sdk.ClientMetadata; +import dev.openfeature.sdk.DefaultHookData; +import dev.openfeature.sdk.ErrorCode; import dev.openfeature.sdk.EvaluationContext; import dev.openfeature.sdk.EventProvider; import dev.openfeature.sdk.FeatureProvider; +import dev.openfeature.sdk.FlagEvaluationDetails; +import dev.openfeature.sdk.FlagValueType; +import dev.openfeature.sdk.Hook; +import dev.openfeature.sdk.HookContext; +import dev.openfeature.sdk.HookData; +import dev.openfeature.sdk.ImmutableContext; import dev.openfeature.sdk.Metadata; import dev.openfeature.sdk.ProviderEvaluation; +import dev.openfeature.sdk.ProviderEvent; +import dev.openfeature.sdk.ProviderEventDetails; +import dev.openfeature.sdk.ProviderState; +import dev.openfeature.sdk.TrackingEventDetails; import dev.openfeature.sdk.Value; +import dev.openfeature.sdk.exceptions.ExceptionUtils; +import dev.openfeature.sdk.exceptions.OpenFeatureError; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.ArrayList; import java.util.Collection; @@ -16,9 +31,12 @@ import java.util.Map; import java.util.Objects; import java.util.concurrent.Callable; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.function.BiConsumer; +import java.util.function.BiFunction; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -40,6 +58,12 @@ public class MultiProvider extends EventProvider { private final Map providers; private final Strategy strategy; + private final Map providerStates = new ConcurrentHashMap<>(); + private final Map> providerEventObservers = + new ConcurrentHashMap<>(); + private final ClientMetadata hookClientMetadata = MultiProvider::getNAME; + private final Map emptyHookHints = Collections.emptyMap(); + private ProviderState aggregateState; private MultiProviderMetadata metadata; /** @@ -61,20 +85,61 @@ public MultiProvider(List providers) { public MultiProvider(List providers, Strategy strategy) { this.providers = buildProviders(providers); this.strategy = Objects.requireNonNull(strategy, "strategy must not be null"); + initializeProviderStates(); + this.aggregateState = determineAggregateState(); } protected static Map buildProviders(List providers) { + Objects.requireNonNull(providers, "providers must not be null"); Map providersMap = new LinkedHashMap<>(providers.size()); + Map suffixesByBaseName = new HashMap<>(providers.size()); for (FeatureProvider provider : providers) { - FeatureProvider prevProvider = - providersMap.put(provider.getMetadata().getName(), provider); - if (prevProvider != null) { - log.info("duplicated provider name: {}", provider.getMetadata().getName()); + Objects.requireNonNull(provider, "provider must not be null"); + String baseName = getProviderBaseName(provider); + String resolvedName = resolveUniqueProviderName(baseName, providersMap, suffixesByBaseName); + if (!baseName.equals(resolvedName)) { + log.info("deduplicated provider name from {} to {}", baseName, resolvedName); } + providersMap.put(resolvedName, provider); } return Collections.unmodifiableMap(providersMap); } + private static String getProviderBaseName(FeatureProvider provider) { + Metadata providerMetadata = provider.getMetadata(); + if (providerMetadata == null || providerMetadata.getName() == null || providerMetadata.getName().isEmpty()) { + return "provider"; + } + return providerMetadata.getName(); + } + + private static String resolveUniqueProviderName( + String baseName, + Map providersMap, + Map suffixesByBaseName) { + if (!providersMap.containsKey(baseName)) { + suffixesByBaseName.putIfAbsent(baseName, 1); + return baseName; + } + int suffix = suffixesByBaseName.getOrDefault(baseName, 1); + String resolvedName = baseName + "-" + suffix; + while (providersMap.containsKey(resolvedName)) { + suffix++; + resolvedName = baseName + "-" + suffix; + } + suffixesByBaseName.put(baseName, suffix + 1); + return resolvedName; + } + + private void initializeProviderStates() { + providerStates.clear(); + if (!providers.isEmpty()) { + for (String providerName : providers.keySet()) { + providerStates.put(providerName, ProviderState.NOT_READY); + } + } + } + /** * Initialize the provider. * @@ -85,7 +150,11 @@ protected static Map buildProviders(List providersMetadata = new HashMap<>(); + Map providersMetadata = new LinkedHashMap<>(); + initializeProviderStates(); + synchronized (this) { + emitAggregateStateChange(determineAggregateState(), ProviderEventDetails.builder().build()); + } if (providers.isEmpty()) { metadataBuilder.originalMetadata(Collections.unmodifiableMap(providersMetadata)); @@ -96,13 +165,22 @@ public void initialize(EvaluationContext evaluationContext) throws Exception { ExecutorService executorService = Executors.newFixedThreadPool(Math.min(INIT_THREADS_COUNT, providers.size())); try { Collection> tasks = new ArrayList<>(providers.size()); - for (FeatureProvider provider : providers.values()) { + for (Map.Entry entry : providers.entrySet()) { + String providerName = entry.getKey(); + FeatureProvider provider = entry.getValue(); + registerChildProviderObserver(providerName, provider); tasks.add(() -> { - provider.initialize(evaluationContext); - return null; + try { + provider.initialize(evaluationContext); + setProviderState(providerName, ProviderState.READY, ProviderEventDetails.builder().build()); + return null; + } catch (Exception e) { + setProviderState(providerName, toStateFromException(e), providerErrorDetails(e)); + throw e; + } }); Metadata providerMetadata = provider.getMetadata(); - providersMetadata.put(providerMetadata.getName(), providerMetadata); + providersMetadata.put(providerName, providerMetadata); } metadataBuilder.originalMetadata(Collections.unmodifiableMap(providersMetadata)); @@ -138,42 +216,470 @@ public Metadata getMetadata() { @Override public ProviderEvaluation getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) { return strategy.evaluate( - providers, key, defaultValue, ctx, p -> p.getBooleanEvaluation(key, defaultValue, ctx)); + providers, + key, + defaultValue, + ctx, + provider -> evaluateWithProviderHooks( + provider, + key, + defaultValue, + ctx, + FlagValueType.BOOLEAN, + (p, evaluationContext) -> p.getBooleanEvaluation(key, defaultValue, evaluationContext))); } @Override public ProviderEvaluation getStringEvaluation(String key, String defaultValue, EvaluationContext ctx) { - return strategy.evaluate(providers, key, defaultValue, ctx, p -> p.getStringEvaluation(key, defaultValue, ctx)); + return strategy.evaluate( + providers, + key, + defaultValue, + ctx, + provider -> evaluateWithProviderHooks( + provider, + key, + defaultValue, + ctx, + FlagValueType.STRING, + (p, evaluationContext) -> p.getStringEvaluation(key, defaultValue, evaluationContext))); } @Override public ProviderEvaluation getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx) { return strategy.evaluate( - providers, key, defaultValue, ctx, p -> p.getIntegerEvaluation(key, defaultValue, ctx)); + providers, + key, + defaultValue, + ctx, + provider -> evaluateWithProviderHooks( + provider, + key, + defaultValue, + ctx, + FlagValueType.INTEGER, + (p, evaluationContext) -> p.getIntegerEvaluation(key, defaultValue, evaluationContext))); } @Override public ProviderEvaluation getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx) { - return strategy.evaluate(providers, key, defaultValue, ctx, p -> p.getDoubleEvaluation(key, defaultValue, ctx)); + return strategy.evaluate( + providers, + key, + defaultValue, + ctx, + provider -> evaluateWithProviderHooks( + provider, + key, + defaultValue, + ctx, + FlagValueType.DOUBLE, + (p, evaluationContext) -> p.getDoubleEvaluation(key, defaultValue, evaluationContext))); } @Override public ProviderEvaluation getObjectEvaluation(String key, Value defaultValue, EvaluationContext ctx) { - return strategy.evaluate(providers, key, defaultValue, ctx, p -> p.getObjectEvaluation(key, defaultValue, ctx)); + return strategy.evaluate( + providers, + key, + defaultValue, + ctx, + provider -> evaluateWithProviderHooks( + provider, + key, + defaultValue, + ctx, + FlagValueType.OBJECT, + (p, evaluationContext) -> p.getObjectEvaluation(key, defaultValue, evaluationContext))); + } + + @Override + public void track(String eventName, EvaluationContext context, TrackingEventDetails details) { + for (Map.Entry entry : providers.entrySet()) { + String providerName = entry.getKey(); + FeatureProvider provider = entry.getValue(); + if (!shouldTrackProvider(providerName)) { + continue; + } + try { + provider.track(eventName, context, details); + } catch (Exception e) { + log.error("error forwarding track to provider {}", providerName, e); + } + } } @Override public void shutdown() { log.debug("shutdown begin"); - for (FeatureProvider provider : providers.values()) { + for (Map.Entry entry : providers.entrySet()) { + String providerName = entry.getKey(); + FeatureProvider provider = entry.getValue(); try { + unregisterChildProviderObserver(providerName, provider); provider.shutdown(); } catch (Exception e) { - log.error("error shutdown provider {}", provider.getMetadata().getName(), e); + log.error("error shutdown provider {}", providerName, e); } } + synchronized (this) { + initializeProviderStates(); + emitAggregateStateChange(ProviderState.NOT_READY, ProviderEventDetails.builder().build()); + } log.debug("shutdown end"); - // Important: ensure EventProvider's executor is also shut down super.shutdown(); } + + private void registerChildProviderObserver(String providerName, FeatureProvider provider) { + if (provider instanceof EventProvider) { + BiConsumer observer = + (event, details) -> onChildProviderEvent(providerName, event, details); + ((EventProvider) provider).addEventObserver(observer); + providerEventObservers.put(providerName, observer); + } + } + + private void unregisterChildProviderObserver(String providerName, FeatureProvider provider) { + if (provider instanceof EventProvider) { + BiConsumer observer = providerEventObservers.remove(providerName); + if (observer != null) { + ((EventProvider) provider).removeEventObserver(observer); + } + } + } + + private void onChildProviderEvent(String providerName, ProviderEvent event, ProviderEventDetails details) { + if (ProviderEvent.PROVIDER_CONFIGURATION_CHANGED.equals(event)) { + emitProviderConfigurationChanged(details); + return; + } + ProviderState state = toStateFromEvent(event, details); + if (state != null) { + setProviderState(providerName, state, details); + } + } + + private synchronized void setProviderState( + String providerName, + ProviderState providerState, + ProviderEventDetails details) { + providerStates.put(providerName, providerState); + ProviderState aggregate = determineAggregateState(); + emitAggregateStateChange(aggregate, details); + } + + private void emitAggregateStateChange(ProviderState aggregate, ProviderEventDetails details) { + ProviderState previous = aggregateState; + if (previous == aggregate) { + return; + } + aggregateState = aggregate; + switch (aggregate) { + case READY: + emitProviderReady(detailsOrEmpty(details)); + break; + case STALE: + emitProviderStale(detailsOrEmpty(details)); + break; + case ERROR: + emitProviderError(ensureErrorDetails(details, ErrorCode.GENERAL)); + break; + case FATAL: + emitProviderError(ensureErrorDetails(details, ErrorCode.PROVIDER_FATAL)); + break; + case NOT_READY: + break; + default: + break; + } + } + + private ProviderState determineAggregateState() { + if (providerStates.isEmpty()) { + return ProviderState.READY; + } + ProviderState aggregate = ProviderState.READY; + for (ProviderState state : providerStates.values()) { + if (stateSeverity(state) > stateSeverity(aggregate)) { + aggregate = state; + } + } + return aggregate; + } + + private int stateSeverity(ProviderState state) { + if (state == null) { + return 0; + } + switch (state) { + case FATAL: + return 5; + case NOT_READY: + return 4; + case ERROR: + return 3; + case STALE: + return 2; + case READY: + return 1; + default: + return 0; + } + } + + private ProviderEventDetails detailsOrEmpty(ProviderEventDetails details) { + if (details == null) { + return ProviderEventDetails.builder().build(); + } + return details; + } + + private ProviderEventDetails ensureErrorDetails(ProviderEventDetails details, ErrorCode defaultErrorCode) { + if (details == null) { + return ProviderEventDetails.builder().errorCode(defaultErrorCode).build(); + } + if (details.getErrorCode() == null) { + return details.toBuilder().errorCode(defaultErrorCode).build(); + } + return details; + } + + private ProviderState toStateFromEvent(ProviderEvent event, ProviderEventDetails details) { + if (ProviderEvent.PROVIDER_READY.equals(event)) { + return ProviderState.READY; + } + if (ProviderEvent.PROVIDER_STALE.equals(event)) { + return ProviderState.STALE; + } + if (ProviderEvent.PROVIDER_ERROR.equals(event)) { + if (details != null && ErrorCode.PROVIDER_FATAL.equals(details.getErrorCode())) { + return ProviderState.FATAL; + } + return ProviderState.ERROR; + } + return null; + } + + private ProviderState toStateFromException(Exception exception) { + if (exception instanceof OpenFeatureError + && ErrorCode.PROVIDER_FATAL.equals(((OpenFeatureError) exception).getErrorCode())) { + return ProviderState.FATAL; + } + return ProviderState.ERROR; + } + + private ProviderEventDetails providerErrorDetails(Exception exception) { + if (exception instanceof OpenFeatureError) { + ErrorCode errorCode = ((OpenFeatureError) exception).getErrorCode(); + return ProviderEventDetails.builder() + .errorCode(errorCode) + .message(exception.getMessage()) + .build(); + } + return ProviderEventDetails.builder() + .errorCode(ErrorCode.GENERAL) + .message(exception.getMessage()) + .build(); + } + + private boolean shouldTrackProvider(String providerName) { + ProviderState providerState = providerStates.getOrDefault(providerName, ProviderState.READY); + return !ProviderState.NOT_READY.equals(providerState) && !ProviderState.FATAL.equals(providerState); + } + + private EvaluationContext copyEvaluationContext(EvaluationContext context) { + if (context == null) { + return ImmutableContext.EMPTY; + } + String targetingKey = context.getTargetingKey(); + if (targetingKey == null) { + return new ImmutableContext(context.asMap()); + } + return new ImmutableContext(targetingKey, context.asMap()); + } + + private EvaluationContext toProviderContext(EvaluationContext originalContext, EvaluationContext evaluatedContext) { + if (originalContext == null && (evaluatedContext == null || evaluatedContext.isEmpty())) { + return null; + } + return evaluatedContext; + } + + private Exception toEvaluationException(ProviderEvaluation providerEvaluation) { + if (providerEvaluation == null || providerEvaluation.getErrorCode() == null) { + return new RuntimeException("Provider evaluation returned an error"); + } + return ExceptionUtils.instantiateErrorByErrorCode( + providerEvaluation.getErrorCode(), + providerEvaluation.getErrorMessage()); + } + + private HookContext createHookContext( + String key, + FlagValueType valueType, + T defaultValue, + EvaluationContext evaluationContext, + FeatureProvider provider, + HookData hookData) { + return HookContext.builder() + .flagKey(key) + .type(valueType) + .defaultValue(normalizeDefaultValue(valueType, defaultValue)) + .ctx(evaluationContext) + .clientMetadata(hookClientMetadata) + .providerMetadata(provider.getMetadata()) + .hookData(hookData) + .build(); + } + + @SuppressWarnings("unchecked") + private T normalizeDefaultValue(FlagValueType valueType, T defaultValue) { + if (defaultValue != null) { + return defaultValue; + } + switch (valueType) { + case BOOLEAN: + return (T) Boolean.FALSE; + case STRING: + return (T) ""; + case INTEGER: + return (T) Integer.valueOf(0); + case DOUBLE: + return (T) Double.valueOf(0d); + case OBJECT: + return (T) new Value(); + default: + return defaultValue; + } + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + private ProviderEvaluation evaluateWithProviderHooks( + FeatureProvider provider, + String key, + T defaultValue, + EvaluationContext ctx, + FlagValueType valueType, + BiFunction> providerFunction) { + List providerHooks = provider.getProviderHooks(); + if (providerHooks == null || providerHooks.isEmpty()) { + return providerFunction.apply(provider, ctx); + } + + List> hooks = new ArrayList<>(providerHooks.size()); + for (Hook hook : providerHooks) { + if (hook.supportsFlagValueType(valueType)) { + hooks.add(new HookExecution<>(hook, new DefaultHookData())); + } + } + + if (hooks.isEmpty()) { + return providerFunction.apply(provider, ctx); + } + + EvaluationContext evaluatedContext = copyEvaluationContext(ctx); + ProviderEvaluation providerEvaluation = null; + FlagEvaluationDetails details = null; + + try { + for (int i = hooks.size() - 1; i >= 0; i--) { + HookExecution execution = hooks.get(i); + HookContext hookContext = + createHookContext(key, valueType, defaultValue, evaluatedContext, provider, execution.hookData); + var contextUpdate = execution.hook.before(hookContext, emptyHookHints); + if (contextUpdate != null + && contextUpdate.isPresent() + && contextUpdate.get() != hookContext.getCtx() + && !contextUpdate.get().isEmpty()) { + evaluatedContext = evaluatedContext.merge(contextUpdate.get()); + } + } + + providerEvaluation = providerFunction.apply(provider, toProviderContext(ctx, evaluatedContext)); + details = FlagEvaluationDetails.from(providerEvaluation, key); + + if (providerEvaluation.getErrorCode() == null) { + for (HookExecution execution : hooks) { + execution.hook.after( + createHookContext( + key, + valueType, + defaultValue, + evaluatedContext, + provider, + execution.hookData), + details, + emptyHookHints); + } + } else { + Exception providerException = toEvaluationException(providerEvaluation); + for (HookExecution execution : hooks) { + try { + execution.hook.error( + createHookContext( + key, + valueType, + defaultValue, + evaluatedContext, + provider, + execution.hookData), + providerException, + emptyHookHints); + } catch (Exception e) { + log.error("error executing provider hook error stage", e); + } + } + } + + return providerEvaluation; + } catch (Exception e) { + for (HookExecution execution : hooks) { + try { + execution.hook.error( + createHookContext( + key, + valueType, + defaultValue, + evaluatedContext, + provider, + execution.hookData), + e, + emptyHookHints); + } catch (Exception hookError) { + log.error("error executing provider hook error stage", hookError); + } + } + throw e; + } finally { + FlagEvaluationDetails finalDetails = details == null + ? FlagEvaluationDetails.builder().flagKey(key).value(defaultValue).build() + : details; + for (HookExecution execution : hooks) { + try { + execution.hook.finallyAfter( + createHookContext( + key, + valueType, + defaultValue, + evaluatedContext, + provider, + execution.hookData), + finalDetails, + emptyHookHints); + } catch (Exception e) { + log.error("error executing provider hook finally stage", e); + } + } + } + } + + private static final class HookExecution { + private final Hook hook; + private final HookData hookData; + + private HookExecution(Hook hook, HookData hookData) { + this.hook = hook; + this.hookData = hookData; + } + } } diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java new file mode 100644 index 000000000..7938e2b95 --- /dev/null +++ b/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java @@ -0,0 +1,103 @@ +package dev.openfeature.sdk.multiprovider; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import dev.openfeature.sdk.ErrorCode; +import dev.openfeature.sdk.FeatureProvider; +import dev.openfeature.sdk.ProviderEvaluation; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.Test; + +class ComparisonStrategyTest extends BaseStrategyTest { + + @Test + void shouldReturnFallbackResultWhenAllProvidersAgree() { + setupProviderSuccess(mockProvider1, "same"); + setupProviderSuccess(mockProvider2, "same"); + + Map providers = new LinkedHashMap<>(); + providers.put("provider1", mockProvider1); + providers.put("provider2", mockProvider2); + + ComparisonStrategy strategy = new ComparisonStrategy("provider2"); + ProviderEvaluation result = strategy.evaluate( + providers, + FLAG_KEY, + DEFAULT_STRING, + null, + p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); + + assertNotNull(result); + assertEquals("same", result.getValue()); + assertNull(result.getErrorCode()); + } + + @Test + void shouldCallMismatchCallbackAndReturnFallbackResult() { + setupProviderSuccess(mockProvider1, "first"); + setupProviderSuccess(mockProvider2, "second"); + + Map providers = new LinkedHashMap<>(); + providers.put("provider1", mockProvider1); + providers.put("provider2", mockProvider2); + + AtomicInteger callbackCount = new AtomicInteger(); + ComparisonStrategy strategy = new ComparisonStrategy("provider2", (key, evaluations) -> callbackCount.incrementAndGet()); + + ProviderEvaluation result = strategy.evaluate( + providers, + FLAG_KEY, + DEFAULT_STRING, + null, + p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); + + assertEquals("second", result.getValue()); + assertNull(result.getErrorCode()); + assertEquals(1, callbackCount.get()); + } + + @Test + void shouldReturnGeneralErrorWhenAnyProviderFails() { + setupProviderSuccess(mockProvider1, "ok"); + setupProviderError(mockProvider2, ErrorCode.PARSE_ERROR); + + Map providers = new LinkedHashMap<>(); + providers.put("provider1", mockProvider1); + providers.put("provider2", mockProvider2); + + ComparisonStrategy strategy = new ComparisonStrategy("provider1"); + ProviderEvaluation result = strategy.evaluate( + providers, + FLAG_KEY, + DEFAULT_STRING, + null, + p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); + + assertEquals(ErrorCode.GENERAL, result.getErrorCode()); + assertTrue(result.getErrorMessage().contains("provider2")); + } + + @Test + void shouldThrowWhenFallbackProviderIsMissing() { + setupProviderSuccess(mockProvider1, "ok"); + + Map providers = new LinkedHashMap<>(); + providers.put("provider1", mockProvider1); + + ComparisonStrategy strategy = new ComparisonStrategy("provider2"); + assertThrows( + IllegalArgumentException.class, + () -> strategy.evaluate( + providers, + FLAG_KEY, + DEFAULT_STRING, + null, + p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null))); + } +} diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java new file mode 100644 index 000000000..42a3f5027 --- /dev/null +++ b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java @@ -0,0 +1,145 @@ +package dev.openfeature.sdk.multiprovider; + +import static org.awaitility.Awaitility.await; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import dev.openfeature.sdk.Client; +import dev.openfeature.sdk.EvaluationContext; +import dev.openfeature.sdk.EventProvider; +import dev.openfeature.sdk.Metadata; +import dev.openfeature.sdk.OpenFeatureAPI; +import dev.openfeature.sdk.ProviderEvaluation; +import dev.openfeature.sdk.ProviderEventDetails; +import dev.openfeature.sdk.ProviderState; +import dev.openfeature.sdk.TrackingEventDetails; +import dev.openfeature.sdk.Value; +import java.time.Duration; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.Test; + +class MultiProviderEventsAndTrackingTest { + + @Test + void shouldAggregateChildProviderStateAndForwardConfigurationEvents() throws Exception { + TrackingProvider provider1 = new TrackingProvider("provider1"); + TrackingProvider provider2 = new TrackingProvider("provider2"); + MultiProvider multiProvider = new MultiProvider(List.of(provider1, provider2)); + + OpenFeatureAPI api = new TestOpenFeatureAPI(); + api.shutdown(); + try { + api.setProviderAndWait("multiProviderEvents", multiProvider); + Client client = api.getClient("multiProviderEvents"); + + await().atMost(Duration.ofSeconds(2)).until(() -> client.getProviderState() == ProviderState.READY); + + AtomicInteger configurationChangedCount = new AtomicInteger(); + client.onProviderConfigurationChanged(details -> configurationChangedCount.incrementAndGet()); + + provider1.emitProviderConfigurationChanged(ProviderEventDetails.builder().message("changed").build()).await(); + await().atMost(Duration.ofSeconds(2)).until(() -> configurationChangedCount.get() == 1); + + provider1.emitProviderStale(ProviderEventDetails.builder().message("stale").build()).await(); + await().atMost(Duration.ofSeconds(2)).until(() -> client.getProviderState() == ProviderState.STALE); + + provider2.emitProviderError( + ProviderEventDetails.builder().errorCode(dev.openfeature.sdk.ErrorCode.GENERAL).build()) + .await(); + await().atMost(Duration.ofSeconds(2)).until(() -> client.getProviderState() == ProviderState.ERROR); + + provider2.emitProviderReady(ProviderEventDetails.builder().build()).await(); + await().atMost(Duration.ofSeconds(2)).until(() -> client.getProviderState() == ProviderState.STALE); + + provider1.emitProviderReady(ProviderEventDetails.builder().build()).await(); + await().atMost(Duration.ofSeconds(2)).until(() -> client.getProviderState() == ProviderState.READY); + + provider1.emitProviderError( + ProviderEventDetails.builder() + .errorCode(dev.openfeature.sdk.ErrorCode.PROVIDER_FATAL) + .build()) + .await(); + await().atMost(Duration.ofSeconds(2)).until(() -> client.getProviderState() == ProviderState.FATAL); + } finally { + api.shutdown(); + } + } + + @Test + void shouldForwardTrackToReadyProvidersAndSkipFatalProviders() throws Exception { + TrackingProvider provider1 = new TrackingProvider("provider1"); + TrackingProvider provider2 = new TrackingProvider("provider2"); + provider2.throwOnTrack = true; + + MultiProvider multiProvider = new MultiProvider(List.of(provider1, provider2)); + multiProvider.initialize(null); + + multiProvider.track("event1", null, null); + assertEquals(1, provider1.trackCount.get()); + assertEquals(1, provider2.trackCount.get()); + + provider1.emitProviderError( + ProviderEventDetails.builder() + .errorCode(dev.openfeature.sdk.ErrorCode.PROVIDER_FATAL) + .build()) + .await(); + + multiProvider.track("event2", null, null); + assertEquals(1, provider1.trackCount.get()); + assertEquals(2, provider2.trackCount.get()); + } + + static class TrackingProvider extends EventProvider { + private final String name; + private final AtomicInteger trackCount = new AtomicInteger(); + private boolean throwOnTrack; + + TrackingProvider(String name) { + this.name = name; + } + + @Override + public Metadata getMetadata() { + return () -> name; + } + + @Override + public void track(String eventName, EvaluationContext context, TrackingEventDetails details) { + trackCount.incrementAndGet(); + if (throwOnTrack) { + throw new RuntimeException("track failure"); + } + } + + @Override + public ProviderEvaluation getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(Boolean.TRUE).build(); + } + + @Override + public ProviderEvaluation getStringEvaluation(String key, String defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value("value").build(); + } + + @Override + public ProviderEvaluation getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(1).build(); + } + + @Override + public ProviderEvaluation getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(1d).build(); + } + + @Override + public ProviderEvaluation getObjectEvaluation(String key, Value defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(new Value("value")).build(); + } + } + + static class TestOpenFeatureAPI extends OpenFeatureAPI { + TestOpenFeatureAPI() { + super(); + } + } +} diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java new file mode 100644 index 000000000..14a5001eb --- /dev/null +++ b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java @@ -0,0 +1,157 @@ +package dev.openfeature.sdk.multiprovider; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import dev.openfeature.sdk.EvaluationContext; +import dev.openfeature.sdk.EventProvider; +import dev.openfeature.sdk.Hook; +import dev.openfeature.sdk.HookContext; +import dev.openfeature.sdk.Metadata; +import dev.openfeature.sdk.MutableContext; +import dev.openfeature.sdk.ProviderEvaluation; +import dev.openfeature.sdk.Value; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.Test; + +class MultiProviderHooksTest { + + @Test + void shouldExecuteProviderHooksAndKeepPerProviderContextIsolation() throws Exception { + RecordingHook firstHook = new RecordingHook("provider1"); + RecordingHook secondHook = new RecordingHook("provider2"); + + HookedStringProvider provider1 = new HookedStringProvider( + "provider1", + List.of(firstHook), + ProviderEvaluation.builder() + .errorCode(dev.openfeature.sdk.ErrorCode.GENERAL) + .errorMessage("failed") + .build()); + HookedStringProvider provider2 = new HookedStringProvider( + "provider2", + List.of(secondHook), + ProviderEvaluation.builder().value("ok").build()); + + MultiProvider multiProvider = new MultiProvider(List.of(provider1, provider2), new FirstSuccessfulStrategy()); + multiProvider.initialize(null); + + ProviderEvaluation evaluation = multiProvider.getStringEvaluation("flag", "default", null); + + assertEquals("ok", evaluation.getValue()); + + assertEquals(1, firstHook.beforeCount.get()); + assertEquals(0, firstHook.afterCount.get()); + assertEquals(1, firstHook.errorCount.get()); + assertEquals(1, firstHook.finallyCount.get()); + + assertEquals(1, secondHook.beforeCount.get()); + assertEquals(1, secondHook.afterCount.get()); + assertEquals(0, secondHook.errorCount.get()); + assertEquals(1, secondHook.finallyCount.get()); + + assertEquals("provider1", provider1.lastEvaluationContext.getValue("hookOwner").asString()); + assertNull(provider1.lastEvaluationContext.getValue("provider2Marker")); + + assertEquals("provider2", provider2.lastEvaluationContext.getValue("hookOwner").asString()); + assertNull(provider2.lastEvaluationContext.getValue("provider1Marker")); + } + + static class RecordingHook implements Hook { + private final String providerName; + private final AtomicInteger beforeCount = new AtomicInteger(); + private final AtomicInteger afterCount = new AtomicInteger(); + private final AtomicInteger errorCount = new AtomicInteger(); + private final AtomicInteger finallyCount = new AtomicInteger(); + + RecordingHook(String providerName) { + this.providerName = providerName; + } + + @Override + public Optional before(HookContext ctx, Map hints) { + beforeCount.incrementAndGet(); + ctx.getHookData().set("provider", providerName); + return Optional.of(new MutableContext() + .add("hookOwner", providerName) + .add(providerName + "Marker", providerName)); + } + + @Override + public void after( + HookContext ctx, + dev.openfeature.sdk.FlagEvaluationDetails details, + Map hints) { + afterCount.incrementAndGet(); + assertEquals(providerName, ctx.getHookData().get("provider")); + } + + @Override + public void error(HookContext ctx, Exception error, Map hints) { + errorCount.incrementAndGet(); + assertEquals(providerName, ctx.getHookData().get("provider")); + } + + @Override + public void finallyAfter( + HookContext ctx, + dev.openfeature.sdk.FlagEvaluationDetails details, + Map hints) { + finallyCount.incrementAndGet(); + assertEquals(providerName, ctx.getHookData().get("provider")); + } + } + + static class HookedStringProvider extends EventProvider { + private final String name; + private final List hooks; + private final ProviderEvaluation evaluation; + private EvaluationContext lastEvaluationContext; + + HookedStringProvider(String name, List hooks, ProviderEvaluation evaluation) { + this.name = name; + this.hooks = hooks; + this.evaluation = evaluation; + } + + @Override + public Metadata getMetadata() { + return () -> name; + } + + @Override + public List getProviderHooks() { + return hooks; + } + + @Override + public ProviderEvaluation getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + + @Override + public ProviderEvaluation getStringEvaluation(String key, String defaultValue, EvaluationContext ctx) { + lastEvaluationContext = ctx == null ? new MutableContext() : ctx; + return evaluation; + } + + @Override + public ProviderEvaluation getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + + @Override + public ProviderEvaluation getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + + @Override + public ProviderEvaluation getObjectEvaluation(String key, Value defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + } +} diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java index d69cd821f..2678ee5f7 100644 --- a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java +++ b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java @@ -73,7 +73,12 @@ void shouldHandleDuplicateProviderNames() { List providers = new ArrayList<>(2); providers.add(mockProvider1); providers.add(mockProvider2); - assertDoesNotThrow(() -> new MultiProvider(providers).initialize(null)); + MultiProvider multiProvider = new MultiProvider(providers); + assertDoesNotThrow(() -> multiProvider.initialize(null)); + MultiProviderMetadata metadata = (MultiProviderMetadata) multiProvider.getMetadata(); + assertEquals(2, metadata.getOriginalMetadata().size()); + assertNotNull(metadata.getOriginalMetadata().get("provider")); + assertNotNull(metadata.getOriginalMetadata().get("provider-1")); } @Test From 05f0fce07f000fab929e3a2cf9837539cce1ed71 Mon Sep 17 00:00:00 2001 From: Jonathan Norris Date: Mon, 16 Mar 2026 14:25:36 -0400 Subject: [PATCH 3/8] fix: address review feedback for multiprovider parity - Decouple OpenFeatureClient from MultiProvider by using a provider-level before hook for context capture (matching JS SDK WeakMap pattern) - Fix ComparisonStrategy to reuse shared ForkJoinPool.commonPool() instead of creating a new thread pool per evaluation call - Add timeout support and custom ExecutorService constructor to ComparisonStrategy - Fix checkstyle violations (Javadoc, line length, import ordering) - Improve type safety in normalizeDefaultValue - Add tests for concurrent evaluation, executor reuse, multi-error collection, and provider hook context capture Signed-off-by: Jonathan Norris --- .../openfeature/sdk/OpenFeatureClient.java | 5 +- .../sdk/multiprovider/ComparisonStrategy.java | 140 ++++++++++++---- .../sdk/multiprovider/MultiProvider.java | 156 ++++++++++++++++-- .../multiprovider/ComparisonStrategyTest.java | 119 ++++++++++++- .../MultiProviderEventsAndTrackingTest.java | 44 +++++ .../multiprovider/MultiProviderHooksTest.java | 110 +++++++++++- 6 files changed, 525 insertions(+), 49 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 0d5d0e643..c5bb0500f 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -203,8 +203,9 @@ private FlagEvaluationDetails evaluateFlag( throw new FatalError("Provider is in an irrecoverable error state"); } - var providerEval = (ProviderEvaluation) - createProviderEvaluation(type, key, defaultValue, provider, hookSupportData.getEvaluationContext()); + var providerEval = (ProviderEvaluation) createProviderEvaluation( + type, key, defaultValue, provider, + hookSupportData.getEvaluationContext()); details = FlagEvaluationDetails.from(providerEval, key); if (details.getErrorCode() != null) { diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java b/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java index d2ba63aca..ef82769fc 100644 --- a/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java +++ b/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java @@ -13,8 +13,9 @@ import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; +import java.util.concurrent.ForkJoinPool; import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import java.util.function.BiConsumer; import java.util.function.Function; import lombok.Getter; @@ -22,19 +23,32 @@ /** * Comparison strategy. * - *

Evaluates all providers and compares successful results. + *

Evaluates all providers in parallel and compares successful results. + * If all providers agree on the value, the fallback provider's result is returned. + * If providers disagree, the optional {@code onMismatch} callback is invoked + * and the fallback provider's result is returned. + * If any provider returns an error, all errors are collected and a + * {@link ErrorCode#GENERAL} error is returned. */ public class ComparisonStrategy implements Strategy { + private static final long DEFAULT_TIMEOUT_MS = 30_000; + @Getter private final String fallbackProvider; private final BiConsumer>> onMismatch; + private final ExecutorService executorService; + private final boolean ownsExecutorService; + private final long timeoutMs; /** * Constructs a comparison strategy with a fallback provider. * - * @param fallbackProvider provider name to use as fallback when successful providers disagree + *

Uses a shared {@link ForkJoinPool#commonPool()} for parallel evaluation. + * + * @param fallbackProvider provider name to use as fallback when successful + * providers disagree */ public ComparisonStrategy(String fallbackProvider) { this(fallbackProvider, null); @@ -43,14 +57,53 @@ public ComparisonStrategy(String fallbackProvider) { /** * Constructs a comparison strategy with fallback provider and mismatch callback. * - * @param fallbackProvider provider name to use as fallback when successful providers disagree - * @param onMismatch callback invoked with all successful evaluations when they disagree + *

Uses a shared {@link ForkJoinPool#commonPool()} for parallel evaluation. + * + * @param fallbackProvider provider name to use as fallback when successful + * providers disagree + * @param onMismatch callback invoked with all successful evaluations + * when they disagree */ public ComparisonStrategy( String fallbackProvider, BiConsumer>> onMismatch) { - this.fallbackProvider = Objects.requireNonNull(fallbackProvider, "fallbackProvider must not be null"); + this(fallbackProvider, onMismatch, ForkJoinPool.commonPool(), + false, DEFAULT_TIMEOUT_MS); + } + + /** + * Constructs a comparison strategy with a caller-supplied executor. + * + * @param fallbackProvider provider name to use as fallback when successful + * providers disagree + * @param onMismatch callback invoked with all successful evaluations + * when they disagree (may be {@code null}) + * @param executorService executor to use for parallel evaluation + * @param timeoutMs maximum time in milliseconds to wait for all + * providers to complete + */ + public ComparisonStrategy( + String fallbackProvider, + BiConsumer>> onMismatch, + ExecutorService executorService, + long timeoutMs) { + this(fallbackProvider, onMismatch, executorService, + false, timeoutMs); + } + + private ComparisonStrategy( + String fallbackProvider, + BiConsumer>> onMismatch, + ExecutorService executorService, + boolean ownsExecutorService, + long timeoutMs) { + this.fallbackProvider = Objects.requireNonNull( + fallbackProvider, "fallbackProvider must not be null"); this.onMismatch = onMismatch; + this.executorService = Objects.requireNonNull( + executorService, "executorService must not be null"); + this.ownsExecutorService = ownsExecutorService; + this.timeoutMs = timeoutMs; } @Override @@ -67,60 +120,85 @@ public ProviderEvaluation evaluate( .build(); } if (!providers.containsKey(fallbackProvider)) { - throw new IllegalArgumentException("fallbackProvider not found in providers: " + fallbackProvider); + throw new IllegalArgumentException( + "fallbackProvider not found in providers: " + + fallbackProvider); } - Map> successfulResults = new ConcurrentHashMap<>(providers.size()); - Map providerErrors = new ConcurrentHashMap<>(providers.size()); - ExecutorService executorService = Executors.newFixedThreadPool(providers.size()); + Map> successfulResults = + new ConcurrentHashMap<>(providers.size()); + Map providerErrors = + new ConcurrentHashMap<>(providers.size()); + try { List> tasks = new ArrayList<>(providers.size()); - for (Map.Entry entry : providers.entrySet()) { + for (Map.Entry entry + : providers.entrySet()) { String providerName = entry.getKey(); FeatureProvider provider = entry.getValue(); tasks.add(() -> { try { - ProviderEvaluation evaluation = providerFunction.apply(provider); + ProviderEvaluation evaluation = + providerFunction.apply(provider); if (evaluation == null) { - providerErrors.put(providerName, "null evaluation"); + providerErrors.put( + providerName, "null evaluation"); } else if (evaluation.getErrorCode() == null) { - successfulResults.put(providerName, evaluation); + successfulResults.put( + providerName, evaluation); } else { providerErrors.put( providerName, - evaluation.getErrorCode() + ": " + String.valueOf(evaluation.getErrorMessage())); + evaluation.getErrorCode() + ": " + + evaluation.getErrorMessage()); } } catch (Exception e) { - providerErrors.put(providerName, e.getClass().getSimpleName() + ": " + e.getMessage()); + providerErrors.put( + providerName, + e.getClass().getSimpleName() + ": " + + e.getMessage()); } return null; }); } - List> futures = executorService.invokeAll(tasks); + List> futures = + executorService.invokeAll( + tasks, timeoutMs, TimeUnit.MILLISECONDS); for (Future future : futures) { + if (future.isCancelled()) { + return ProviderEvaluation.builder() + .errorCode(ErrorCode.GENERAL) + .errorMessage( + "Comparison strategy timed out after " + + timeoutMs + "ms") + .build(); + } future.get(); } } catch (Exception e) { return ProviderEvaluation.builder() .errorCode(ErrorCode.GENERAL) - .errorMessage("Comparison strategy failed: " + e.getMessage()) + .errorMessage("Comparison strategy failed: " + + e.getMessage()) .build(); - } finally { - executorService.shutdown(); } if (!providerErrors.isEmpty()) { return ProviderEvaluation.builder() .errorCode(ErrorCode.GENERAL) - .errorMessage("Provider errors: " + buildErrorSummary(providerErrors)) + .errorMessage("Provider errors: " + + buildErrorSummary(providerErrors)) .build(); } - ProviderEvaluation fallbackResult = successfulResults.get(fallbackProvider); + ProviderEvaluation fallbackResult = + successfulResults.get(fallbackProvider); if (fallbackResult == null) { return ProviderEvaluation.builder() .errorCode(ErrorCode.GENERAL) - .errorMessage("Fallback provider did not return a successful evaluation: " + fallbackProvider) + .errorMessage( + "Fallback provider did not return a successful " + + "evaluation: " + fallbackProvider) .build(); } @@ -129,8 +207,10 @@ public ProviderEvaluation evaluate( } if (onMismatch != null) { - Map> mismatchPayload = new LinkedHashMap<>(successfulResults); - onMismatch.accept(key, Collections.unmodifiableMap(mismatchPayload)); + Map> mismatchPayload = + new LinkedHashMap<>(successfulResults); + onMismatch.accept( + key, Collections.unmodifiableMap(mismatchPayload)); } return fallbackResult; } @@ -143,19 +223,23 @@ private String buildErrorSummary(Map providerErrors) { builder.append("; "); } first = false; - builder.append(entry.getKey()).append(" -> ").append(entry.getValue()); + builder.append(entry.getKey()) + .append(" -> ") + .append(entry.getValue()); } return builder.toString(); } - private boolean allEvaluationsMatch(Map> results) { + private boolean allEvaluationsMatch( + Map> results) { ProviderEvaluation baseline = null; for (ProviderEvaluation evaluation : results.values()) { if (baseline == null) { baseline = evaluation; continue; } - if (!Objects.equals(baseline.getValue(), evaluation.getValue())) { + if (!Objects.equals( + baseline.getValue(), evaluation.getValue())) { return false; } } diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java index 937a10458..b5b8822df 100644 --- a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java +++ b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java @@ -17,6 +17,7 @@ import dev.openfeature.sdk.ProviderEvent; import dev.openfeature.sdk.ProviderEventDetails; import dev.openfeature.sdk.ProviderState; +import dev.openfeature.sdk.Reason; import dev.openfeature.sdk.TrackingEventDetails; import dev.openfeature.sdk.Value; import dev.openfeature.sdk.exceptions.ExceptionUtils; @@ -30,6 +31,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; @@ -61,6 +63,8 @@ public class MultiProvider extends EventProvider { private final Map providerStates = new ConcurrentHashMap<>(); private final Map> providerEventObservers = new ConcurrentHashMap<>(); + private final ThreadLocal hookExecutionContextThreadLocal = + new ThreadLocal<>(); private final ClientMetadata hookClientMetadata = MultiProvider::getNAME; private final Map emptyHookHints = Collections.emptyMap(); private ProviderState aggregateState; @@ -89,6 +93,38 @@ public MultiProvider(List providers, Strategy strategy) { this.aggregateState = determineAggregateState(); } + /** + * Returns provider-level hooks for this MultiProvider. + * + *

Includes a {@code before} hook that captures the {@link ClientMetadata} + * and hook hints from the SDK's hook lifecycle. This context is then available + * during per-child-provider hook execution, matching the JS SDK's WeakMap-based + * approach for passing hook context into the provider evaluation. + * + * @return the list of provider hooks + */ + @SuppressWarnings({"rawtypes", "unchecked"}) + @Override + public List getProviderHooks() { + Hook contextCapturingHook = new Hook() { + @Override + public Optional before(HookContext ctx, Map hints) { + hookExecutionContextThreadLocal.set( + new HookExecutionContext( + ctx.getClientMetadata(), + hints != null ? hints : emptyHookHints)); + return Optional.empty(); + } + + @Override + public void finallyAfter(HookContext ctx, FlagEvaluationDetails details, + Map hints) { + hookExecutionContextThreadLocal.remove(); + } + }; + return List.of(contextCapturingHook); + } + protected static Map buildProviders(List providers) { Objects.requireNonNull(providers, "providers must not be null"); Map providersMap = new LinkedHashMap<>(providers.size()); @@ -172,7 +208,7 @@ public void initialize(EvaluationContext evaluationContext) throws Exception { tasks.add(() -> { try { provider.initialize(evaluationContext); - setProviderState(providerName, ProviderState.READY, ProviderEventDetails.builder().build()); + setProviderReadyIfStillNotReady(providerName); return null; } catch (Exception e) { setProviderState(providerName, toStateFromException(e), providerErrorDetails(e)); @@ -215,6 +251,7 @@ public Metadata getMetadata() { @Override public ProviderEvaluation getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) { + HookExecutionContext hookExecutionContext = currentHookExecutionContext(); return strategy.evaluate( providers, key, @@ -225,12 +262,14 @@ public ProviderEvaluation getBooleanEvaluation(String key, Boolean defa key, defaultValue, ctx, + hookExecutionContext, FlagValueType.BOOLEAN, (p, evaluationContext) -> p.getBooleanEvaluation(key, defaultValue, evaluationContext))); } @Override public ProviderEvaluation getStringEvaluation(String key, String defaultValue, EvaluationContext ctx) { + HookExecutionContext hookExecutionContext = currentHookExecutionContext(); return strategy.evaluate( providers, key, @@ -241,12 +280,14 @@ public ProviderEvaluation getStringEvaluation(String key, String default key, defaultValue, ctx, + hookExecutionContext, FlagValueType.STRING, (p, evaluationContext) -> p.getStringEvaluation(key, defaultValue, evaluationContext))); } @Override public ProviderEvaluation getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx) { + HookExecutionContext hookExecutionContext = currentHookExecutionContext(); return strategy.evaluate( providers, key, @@ -257,12 +298,14 @@ public ProviderEvaluation getIntegerEvaluation(String key, Integer defa key, defaultValue, ctx, + hookExecutionContext, FlagValueType.INTEGER, (p, evaluationContext) -> p.getIntegerEvaluation(key, defaultValue, evaluationContext))); } @Override public ProviderEvaluation getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx) { + HookExecutionContext hookExecutionContext = currentHookExecutionContext(); return strategy.evaluate( providers, key, @@ -273,12 +316,14 @@ public ProviderEvaluation getDoubleEvaluation(String key, Double default key, defaultValue, ctx, + hookExecutionContext, FlagValueType.DOUBLE, (p, evaluationContext) -> p.getDoubleEvaluation(key, defaultValue, evaluationContext))); } @Override public ProviderEvaluation getObjectEvaluation(String key, Value defaultValue, EvaluationContext ctx) { + HookExecutionContext hookExecutionContext = currentHookExecutionContext(); return strategy.evaluate( providers, key, @@ -289,6 +334,7 @@ public ProviderEvaluation getObjectEvaluation(String key, Value defaultVa key, defaultValue, ctx, + hookExecutionContext, FlagValueType.OBJECT, (p, evaluationContext) -> p.getObjectEvaluation(key, defaultValue, evaluationContext))); } @@ -368,6 +414,15 @@ private synchronized void setProviderState( emitAggregateStateChange(aggregate, details); } + private synchronized void setProviderReadyIfStillNotReady(String providerName) { + if (!ProviderState.NOT_READY.equals(providerStates.get(providerName))) { + return; + } + providerStates.put(providerName, ProviderState.READY); + ProviderState aggregate = determineAggregateState(); + emitAggregateStateChange(aggregate, ProviderEventDetails.builder().build()); + } + private void emitAggregateStateChange(ProviderState aggregate, ProviderEventDetails details) { ProviderState previous = aggregateState; if (previous == aggregate) { @@ -514,43 +569,58 @@ private Exception toEvaluationException(ProviderEvaluation providerEvaluation providerEvaluation.getErrorMessage()); } + @SuppressWarnings("deprecation") private HookContext createHookContext( String key, FlagValueType valueType, T defaultValue, EvaluationContext evaluationContext, FeatureProvider provider, + HookExecutionContext hookExecutionContext, HookData hookData) { return HookContext.builder() .flagKey(key) .type(valueType) .defaultValue(normalizeDefaultValue(valueType, defaultValue)) .ctx(evaluationContext) - .clientMetadata(hookClientMetadata) + .clientMetadata(resolveClientMetadata(hookExecutionContext)) .providerMetadata(provider.getMetadata()) .hookData(hookData) .build(); } + /** + * Returns a non-null default value for use in hook contexts when the caller + * passes {@code null}. The returned object is always assignment-compatible + * with the expected type for {@code valueType}. + */ @SuppressWarnings("unchecked") private T normalizeDefaultValue(FlagValueType valueType, T defaultValue) { if (defaultValue != null) { return defaultValue; } + Object fallback; switch (valueType) { case BOOLEAN: - return (T) Boolean.FALSE; + fallback = Boolean.FALSE; + break; case STRING: - return (T) ""; + fallback = ""; + break; case INTEGER: - return (T) Integer.valueOf(0); + fallback = Integer.valueOf(0); + break; case DOUBLE: - return (T) Double.valueOf(0d); + fallback = Double.valueOf(0d); + break; case OBJECT: - return (T) new Value(); + fallback = new Value(); + break; default: return defaultValue; } + // Safe: the SDK guarantees T matches the valueType enum. + return (T) fallback; } @SuppressWarnings({"rawtypes", "unchecked"}) @@ -559,6 +629,7 @@ private ProviderEvaluation evaluateWithProviderHooks( String key, T defaultValue, EvaluationContext ctx, + HookExecutionContext hookExecutionContext, FlagValueType valueType, BiFunction> providerFunction) { List providerHooks = provider.getProviderHooks(); @@ -580,13 +651,16 @@ private ProviderEvaluation evaluateWithProviderHooks( EvaluationContext evaluatedContext = copyEvaluationContext(ctx); ProviderEvaluation providerEvaluation = null; FlagEvaluationDetails details = null; + Map hookHints = resolveHookHints(hookExecutionContext); try { for (int i = hooks.size() - 1; i >= 0; i--) { HookExecution execution = hooks.get(i); - HookContext hookContext = - createHookContext(key, valueType, defaultValue, evaluatedContext, provider, execution.hookData); - var contextUpdate = execution.hook.before(hookContext, emptyHookHints); + HookContext hookContext = createHookContext( + key, valueType, defaultValue, + evaluatedContext, provider, + hookExecutionContext, execution.hookData); + var contextUpdate = execution.hook.before(hookContext, hookHints); if (contextUpdate != null && contextUpdate.isPresent() && contextUpdate.get() != hookContext.getCtx() @@ -607,11 +681,13 @@ private ProviderEvaluation evaluateWithProviderHooks( defaultValue, evaluatedContext, provider, + hookExecutionContext, execution.hookData), details, - emptyHookHints); + hookHints); } } else { + enrichDetailsWithErrorDefaults(defaultValue, details); Exception providerException = toEvaluationException(providerEvaluation); for (HookExecution execution : hooks) { try { @@ -622,9 +698,10 @@ private ProviderEvaluation evaluateWithProviderHooks( defaultValue, evaluatedContext, provider, + hookExecutionContext, execution.hookData), providerException, - emptyHookHints); + hookHints); } catch (Exception e) { log.error("error executing provider hook error stage", e); } @@ -633,6 +710,7 @@ private ProviderEvaluation evaluateWithProviderHooks( return providerEvaluation; } catch (Exception e) { + details = buildErrorDetails(key, defaultValue, details, e); for (HookExecution execution : hooks) { try { execution.hook.error( @@ -642,9 +720,10 @@ private ProviderEvaluation evaluateWithProviderHooks( defaultValue, evaluatedContext, provider, + hookExecutionContext, execution.hookData), e, - emptyHookHints); + hookHints); } catch (Exception hookError) { log.error("error executing provider hook error stage", hookError); } @@ -663,9 +742,10 @@ private ProviderEvaluation evaluateWithProviderHooks( defaultValue, evaluatedContext, provider, + hookExecutionContext, execution.hookData), finalDetails, - emptyHookHints); + hookHints); } catch (Exception e) { log.error("error executing provider hook finally stage", e); } @@ -673,6 +753,44 @@ private ProviderEvaluation evaluateWithProviderHooks( } } + private HookExecutionContext currentHookExecutionContext() { + return hookExecutionContextThreadLocal.get(); + } + + private ClientMetadata resolveClientMetadata(HookExecutionContext hookExecutionContext) { + if (hookExecutionContext == null || hookExecutionContext.clientMetadata == null) { + return hookClientMetadata; + } + return hookExecutionContext.clientMetadata; + } + + private Map resolveHookHints(HookExecutionContext hookExecutionContext) { + if (hookExecutionContext == null || hookExecutionContext.hints == null) { + return emptyHookHints; + } + return hookExecutionContext.hints; + } + + private FlagEvaluationDetails buildErrorDetails( + String key, T defaultValue, FlagEvaluationDetails details, Exception error) { + FlagEvaluationDetails errorDetails = details == null + ? FlagEvaluationDetails.builder().flagKey(key).build() + : details; + if (error instanceof OpenFeatureError) { + errorDetails.setErrorCode(((OpenFeatureError) error).getErrorCode()); + } else { + errorDetails.setErrorCode(ErrorCode.GENERAL); + } + errorDetails.setErrorMessage(error.getMessage()); + enrichDetailsWithErrorDefaults(defaultValue, errorDetails); + return errorDetails; + } + + private void enrichDetailsWithErrorDefaults(T defaultValue, FlagEvaluationDetails details) { + details.setValue(defaultValue); + details.setReason(Reason.ERROR.toString()); + } + private static final class HookExecution { private final Hook hook; private final HookData hookData; @@ -682,4 +800,14 @@ private HookExecution(Hook hook, HookData hookData) { this.hookData = hookData; } } + + private static final class HookExecutionContext { + private final ClientMetadata clientMetadata; + private final Map hints; + + private HookExecutionContext(ClientMetadata clientMetadata, Map hints) { + this.clientMetadata = clientMetadata; + this.hints = hints; + } + } } diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java index 7938e2b95..72628dfde 100644 --- a/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java +++ b/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java @@ -11,6 +11,12 @@ import dev.openfeature.sdk.ProviderEvaluation; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Test; @@ -48,7 +54,9 @@ void shouldCallMismatchCallbackAndReturnFallbackResult() { providers.put("provider2", mockProvider2); AtomicInteger callbackCount = new AtomicInteger(); - ComparisonStrategy strategy = new ComparisonStrategy("provider2", (key, evaluations) -> callbackCount.incrementAndGet()); + ComparisonStrategy strategy = new ComparisonStrategy( + "provider2", + (key, evaluations) -> callbackCount.incrementAndGet()); ProviderEvaluation result = strategy.evaluate( providers, @@ -100,4 +108,113 @@ void shouldThrowWhenFallbackProviderIsMissing() { null, p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null))); } + + @Test + void shouldEvaluateProvidersConcurrently() throws InterruptedException { + // Use a latch to prove that providers run in parallel: + // both providers block on the latch, so they must be on + // separate threads for the test to complete. + CountDownLatch bothStarted = new CountDownLatch(2); + CountDownLatch proceed = new CountDownLatch(1); + Set threadNames = ConcurrentHashMap.newKeySet(); + + Map providers = new LinkedHashMap<>(); + providers.put("provider1", mockProvider1); + providers.put("provider2", mockProvider2); + + setupProviderSuccess(mockProvider1, "val"); + setupProviderSuccess(mockProvider2, "val"); + + ComparisonStrategy strategy = new ComparisonStrategy("provider1"); + ProviderEvaluation result = strategy.evaluate( + providers, + FLAG_KEY, + DEFAULT_STRING, + null, + provider -> { + threadNames.add(Thread.currentThread().getName()); + bothStarted.countDown(); + try { + // Wait for both providers to signal they've started + bothStarted.await(5, TimeUnit.SECONDS); + proceed.countDown(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + return provider.getStringEvaluation( + FLAG_KEY, DEFAULT_STRING, null); + }); + + assertNotNull(result); + assertEquals("val", result.getValue()); + assertNull(result.getErrorCode()); + // Verify that at least 2 different threads were used + assertTrue( + threadNames.size() >= 2, + "Expected concurrent execution on multiple threads, " + + "but only saw: " + threadNames); + } + + @Test + void shouldReuseProvidedExecutorService() { + ExecutorService customExecutor = Executors.newFixedThreadPool(2); + try { + setupProviderSuccess(mockProvider1, "ok"); + setupProviderSuccess(mockProvider2, "ok"); + + Map providers = new LinkedHashMap<>(); + providers.put("provider1", mockProvider1); + providers.put("provider2", mockProvider2); + + ComparisonStrategy strategy = new ComparisonStrategy( + "provider1", null, customExecutor, 5000); + + // Execute twice to confirm the same executor is reused + for (int i = 0; i < 2; i++) { + ProviderEvaluation result = strategy.evaluate( + providers, + FLAG_KEY, + DEFAULT_STRING, + null, + p -> p.getStringEvaluation( + FLAG_KEY, DEFAULT_STRING, null)); + assertNotNull(result); + assertEquals("ok", result.getValue()); + assertNull(result.getErrorCode()); + } + + // Executor should still be usable (not shut down) + assertTrue( + !customExecutor.isShutdown(), + "Executor should not be shut down by the strategy"); + } finally { + customExecutor.shutdown(); + } + } + + @Test + void shouldCollectAllProviderErrorsWhenMultipleFail() { + setupProviderError(mockProvider1, ErrorCode.PARSE_ERROR); + setupProviderError(mockProvider2, ErrorCode.FLAG_NOT_FOUND); + + Map providers = new LinkedHashMap<>(); + providers.put("provider1", mockProvider1); + providers.put("provider2", mockProvider2); + + ComparisonStrategy strategy = new ComparisonStrategy("provider1"); + ProviderEvaluation result = strategy.evaluate( + providers, + FLAG_KEY, + DEFAULT_STRING, + null, + p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); + + assertEquals(ErrorCode.GENERAL, result.getErrorCode()); + assertTrue( + result.getErrorMessage().contains("provider1"), + "Error should mention provider1"); + assertTrue( + result.getErrorMessage().contains("provider2"), + "Error should mention provider2"); + } } diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java index 42a3f5027..aed14ed49 100644 --- a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java +++ b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java @@ -9,12 +9,14 @@ import dev.openfeature.sdk.Metadata; import dev.openfeature.sdk.OpenFeatureAPI; import dev.openfeature.sdk.ProviderEvaluation; +import dev.openfeature.sdk.ProviderEvent; import dev.openfeature.sdk.ProviderEventDetails; import dev.openfeature.sdk.ProviderState; import dev.openfeature.sdk.TrackingEventDetails; import dev.openfeature.sdk.Value; import java.time.Duration; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Test; @@ -65,6 +67,20 @@ void shouldAggregateChildProviderStateAndForwardConfigurationEvents() throws Exc } } + @Test + void shouldPreserveChildStateEmittedDuringInitialize() throws Exception { + TrackingProvider provider1 = new InitializingStateProvider("provider1", ProviderState.STALE); + TrackingProvider provider2 = new TrackingProvider("provider2"); + MultiProvider multiProvider = new MultiProvider(List.of(provider1, provider2)); + List emittedEvents = new CopyOnWriteArrayList<>(); + multiProvider.addEventObserver((event, details) -> emittedEvents.add(event)); + + multiProvider.initialize(null); + + await().atMost(Duration.ofSeconds(2)).until(() -> emittedEvents.contains(ProviderEvent.PROVIDER_STALE)); + assertEquals(List.of(ProviderEvent.PROVIDER_STALE), emittedEvents); + } + @Test void shouldForwardTrackToReadyProvidersAndSkipFatalProviders() throws Exception { TrackingProvider provider1 = new TrackingProvider("provider1"); @@ -137,6 +153,34 @@ public ProviderEvaluation getObjectEvaluation(String key, Value defaultVa } } + static class InitializingStateProvider extends TrackingProvider { + private final ProviderState initializeState; + + InitializingStateProvider(String name, ProviderState initializeState) { + super(name); + this.initializeState = initializeState; + } + + @Override + public void initialize(EvaluationContext evaluationContext) throws Exception { + if (ProviderState.STALE.equals(initializeState)) { + emitProviderStale(ProviderEventDetails.builder().message("stale during init").build()).await(); + } else if (ProviderState.FATAL.equals(initializeState)) { + emitProviderError(ProviderEventDetails.builder() + .errorCode(dev.openfeature.sdk.ErrorCode.PROVIDER_FATAL) + .message("fatal during init") + .build()) + .await(); + } else if (ProviderState.ERROR.equals(initializeState)) { + emitProviderError(ProviderEventDetails.builder() + .errorCode(dev.openfeature.sdk.ErrorCode.GENERAL) + .message("error during init") + .build()) + .await(); + } + } + } + static class TestOpenFeatureAPI extends OpenFeatureAPI { TestOpenFeatureAPI() { super(); diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java index 14a5001eb..dd4af41e4 100644 --- a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java +++ b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java @@ -1,17 +1,24 @@ package dev.openfeature.sdk.multiprovider; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import dev.openfeature.sdk.Client; +import dev.openfeature.sdk.ErrorCode; import dev.openfeature.sdk.EvaluationContext; import dev.openfeature.sdk.EventProvider; +import dev.openfeature.sdk.FlagEvaluationDetails; +import dev.openfeature.sdk.FlagEvaluationOptions; import dev.openfeature.sdk.Hook; import dev.openfeature.sdk.HookContext; +import dev.openfeature.sdk.ImmutableContext; import dev.openfeature.sdk.Metadata; import dev.openfeature.sdk.MutableContext; +import dev.openfeature.sdk.OpenFeatureAPI; import dev.openfeature.sdk.ProviderEvaluation; +import dev.openfeature.sdk.Reason; import dev.openfeature.sdk.Value; -import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; @@ -56,17 +63,84 @@ void shouldExecuteProviderHooksAndKeepPerProviderContextIsolation() throws Excep assertEquals("provider1", provider1.lastEvaluationContext.getValue("hookOwner").asString()); assertNull(provider1.lastEvaluationContext.getValue("provider2Marker")); + assertNotNull(firstHook.lastFinallyDetails); + assertEquals(ErrorCode.GENERAL, firstHook.lastFinallyDetails.getErrorCode()); + assertEquals(Reason.ERROR.toString(), firstHook.lastFinallyDetails.getReason()); + assertEquals("default", firstHook.lastFinallyDetails.getValue()); + assertEquals("failed", firstHook.lastFinallyDetails.getErrorMessage()); assertEquals("provider2", provider2.lastEvaluationContext.getValue("hookOwner").asString()); assertNull(provider2.lastEvaluationContext.getValue("provider1Marker")); } + @Test + void shouldExposeContextCapturingProviderHook() throws Exception { + HookedStringProvider provider1 = new HookedStringProvider( + "provider1", + List.of(), + ProviderEvaluation.builder().value("ok").build()); + + MultiProvider multiProvider = new MultiProvider( + List.of(provider1), new FirstSuccessfulStrategy()); + multiProvider.initialize(null); + + // MultiProvider should expose a provider hook for context capture + var providerHooks = multiProvider.getProviderHooks(); + assertNotNull(providerHooks); + assertEquals(1, providerHooks.size(), "Should have exactly one context-capturing hook"); + } + + @Test + void shouldPassHookHintsAndClientMetadataAndEnrichThrownProviderErrors() throws Exception { + RecordingHook firstHook = new RecordingHook("provider1"); + RecordingHook secondHook = new RecordingHook("provider2"); + + HookedStringProvider provider1 = new HookedStringProvider("provider1", List.of(firstHook), new RuntimeException("boom")); + HookedStringProvider provider2 = new HookedStringProvider( + "provider2", + List.of(secondHook), + ProviderEvaluation.builder().value("ok").build()); + + MultiProvider multiProvider = new MultiProvider(List.of(provider1, provider2), new FirstSuccessfulStrategy()); + + OpenFeatureAPI api = new TestOpenFeatureAPI(); + api.shutdown(); + try { + api.setProviderAndWait("multiProviderHooks", multiProvider); + Client client = api.getClient("multiProviderHooks"); + + var evaluation = client.getStringDetails( + "flag", + "default", + new ImmutableContext(), + FlagEvaluationOptions.builder().hookHints(Map.of("hintKey", "hintValue")).build()); + + assertEquals("ok", evaluation.getValue()); + + assertEquals("hintValue", firstHook.lastHints.get("hintKey")); + assertEquals("hintValue", secondHook.lastHints.get("hintKey")); + assertEquals("multiProviderHooks", firstHook.lastClientDomain); + assertEquals("multiProviderHooks", secondHook.lastClientDomain); + + assertNotNull(firstHook.lastFinallyDetails); + assertEquals(ErrorCode.GENERAL, firstHook.lastFinallyDetails.getErrorCode()); + assertEquals(Reason.ERROR.toString(), firstHook.lastFinallyDetails.getReason()); + assertEquals("default", firstHook.lastFinallyDetails.getValue()); + assertEquals("boom", firstHook.lastFinallyDetails.getErrorMessage()); + } finally { + api.shutdown(); + } + } + static class RecordingHook implements Hook { private final String providerName; private final AtomicInteger beforeCount = new AtomicInteger(); private final AtomicInteger afterCount = new AtomicInteger(); private final AtomicInteger errorCount = new AtomicInteger(); private final AtomicInteger finallyCount = new AtomicInteger(); + private Map lastHints = Map.of(); + private String lastClientDomain; + private FlagEvaluationDetails lastFinallyDetails; RecordingHook(String providerName) { this.providerName = providerName; @@ -76,6 +150,8 @@ static class RecordingHook implements Hook { public Optional before(HookContext ctx, Map hints) { beforeCount.incrementAndGet(); ctx.getHookData().set("provider", providerName); + lastHints = hints; + lastClientDomain = ctx.getClientMetadata().getDomain(); return Optional.of(new MutableContext() .add("hookOwner", providerName) .add(providerName + "Marker", providerName)); @@ -88,12 +164,16 @@ public void after( Map hints) { afterCount.incrementAndGet(); assertEquals(providerName, ctx.getHookData().get("provider")); + lastHints = hints; + lastClientDomain = ctx.getClientMetadata().getDomain(); } @Override public void error(HookContext ctx, Exception error, Map hints) { errorCount.incrementAndGet(); assertEquals(providerName, ctx.getHookData().get("provider")); + lastHints = hints; + lastClientDomain = ctx.getClientMetadata().getDomain(); } @Override @@ -103,19 +183,31 @@ public void finallyAfter( Map hints) { finallyCount.incrementAndGet(); assertEquals(providerName, ctx.getHookData().get("provider")); + lastHints = hints; + lastClientDomain = ctx.getClientMetadata().getDomain(); + lastFinallyDetails = details; } } static class HookedStringProvider extends EventProvider { private final String name; - private final List hooks; + private final List> hooks; private final ProviderEvaluation evaluation; + private final RuntimeException evaluationException; private EvaluationContext lastEvaluationContext; - HookedStringProvider(String name, List hooks, ProviderEvaluation evaluation) { + HookedStringProvider(String name, List> hooks, ProviderEvaluation evaluation) { this.name = name; this.hooks = hooks; this.evaluation = evaluation; + this.evaluationException = null; + } + + HookedStringProvider(String name, List> hooks, RuntimeException evaluationException) { + this.name = name; + this.hooks = hooks; + this.evaluation = null; + this.evaluationException = evaluationException; } @Override @@ -124,8 +216,9 @@ public Metadata getMetadata() { } @Override + @SuppressWarnings("rawtypes") public List getProviderHooks() { - return hooks; + return List.copyOf(hooks); } @Override @@ -136,6 +229,9 @@ public ProviderEvaluation getBooleanEvaluation(String key, Boolean defa @Override public ProviderEvaluation getStringEvaluation(String key, String defaultValue, EvaluationContext ctx) { lastEvaluationContext = ctx == null ? new MutableContext() : ctx; + if (evaluationException != null) { + throw evaluationException; + } return evaluation; } @@ -154,4 +250,10 @@ public ProviderEvaluation getObjectEvaluation(String key, Value defaultVa return ProviderEvaluation.builder().value(defaultValue).build(); } } + + static class TestOpenFeatureAPI extends OpenFeatureAPI { + TestOpenFeatureAPI() { + super(); + } + } } From eebb7fa0857a7c54350f9f7ce475a13fb98667ae Mon Sep 17 00:00:00 2001 From: Jonathan Norris Date: Fri, 8 May 2026 14:31:22 -0400 Subject: [PATCH 4/8] fix: spotless formatting, sonar reliability issues, remove dead ownsExecutorService field Signed-off-by: Jonathan Norris --- .../openfeature/sdk/OpenFeatureClient.java | 5 +- .../sdk/multiprovider/ComparisonStrategy.java | 98 ++++++------------- .../sdk/multiprovider/MultiProvider.java | 48 ++++----- .../multiprovider/ComparisonStrategyTest.java | 80 +++++---------- .../MultiProviderEventsAndTrackingTest.java | 43 +++++--- .../multiprovider/MultiProviderHooksTest.java | 29 +++--- 6 files changed, 127 insertions(+), 176 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index c5bb0500f..0d5d0e643 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -203,9 +203,8 @@ private FlagEvaluationDetails evaluateFlag( throw new FatalError("Provider is in an irrecoverable error state"); } - var providerEval = (ProviderEvaluation) createProviderEvaluation( - type, key, defaultValue, provider, - hookSupportData.getEvaluationContext()); + var providerEval = (ProviderEvaluation) + createProviderEvaluation(type, key, defaultValue, provider, hookSupportData.getEvaluationContext()); details = FlagEvaluationDetails.from(providerEval, key); if (details.getErrorCode() != null) { diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java b/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java index ef82769fc..672dd7a82 100644 --- a/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java +++ b/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java @@ -39,7 +39,6 @@ public class ComparisonStrategy implements Strategy { private final BiConsumer>> onMismatch; private final ExecutorService executorService; - private final boolean ownsExecutorService; private final long timeoutMs; /** @@ -65,10 +64,8 @@ public ComparisonStrategy(String fallbackProvider) { * when they disagree */ public ComparisonStrategy( - String fallbackProvider, - BiConsumer>> onMismatch) { - this(fallbackProvider, onMismatch, ForkJoinPool.commonPool(), - false, DEFAULT_TIMEOUT_MS); + String fallbackProvider, BiConsumer>> onMismatch) { + this(fallbackProvider, onMismatch, ForkJoinPool.commonPool(), DEFAULT_TIMEOUT_MS); } /** @@ -87,22 +84,9 @@ public ComparisonStrategy( BiConsumer>> onMismatch, ExecutorService executorService, long timeoutMs) { - this(fallbackProvider, onMismatch, executorService, - false, timeoutMs); - } - - private ComparisonStrategy( - String fallbackProvider, - BiConsumer>> onMismatch, - ExecutorService executorService, - boolean ownsExecutorService, - long timeoutMs) { - this.fallbackProvider = Objects.requireNonNull( - fallbackProvider, "fallbackProvider must not be null"); + this.fallbackProvider = Objects.requireNonNull(fallbackProvider, "fallbackProvider must not be null"); this.onMismatch = onMismatch; - this.executorService = Objects.requireNonNull( - executorService, "executorService must not be null"); - this.ownsExecutorService = ownsExecutorService; + this.executorService = Objects.requireNonNull(executorService, "executorService must not be null"); this.timeoutMs = timeoutMs; } @@ -120,85 +104,69 @@ public ProviderEvaluation evaluate( .build(); } if (!providers.containsKey(fallbackProvider)) { - throw new IllegalArgumentException( - "fallbackProvider not found in providers: " - + fallbackProvider); + throw new IllegalArgumentException("fallbackProvider not found in providers: " + fallbackProvider); } - Map> successfulResults = - new ConcurrentHashMap<>(providers.size()); - Map providerErrors = - new ConcurrentHashMap<>(providers.size()); + Map> successfulResults = new ConcurrentHashMap<>(providers.size()); + Map providerErrors = new ConcurrentHashMap<>(providers.size()); try { List> tasks = new ArrayList<>(providers.size()); - for (Map.Entry entry - : providers.entrySet()) { + for (Map.Entry entry : providers.entrySet()) { String providerName = entry.getKey(); FeatureProvider provider = entry.getValue(); tasks.add(() -> { try { - ProviderEvaluation evaluation = - providerFunction.apply(provider); + ProviderEvaluation evaluation = providerFunction.apply(provider); if (evaluation == null) { - providerErrors.put( - providerName, "null evaluation"); + providerErrors.put(providerName, "null evaluation"); } else if (evaluation.getErrorCode() == null) { - successfulResults.put( - providerName, evaluation); + successfulResults.put(providerName, evaluation); } else { providerErrors.put( - providerName, - evaluation.getErrorCode() + ": " - + evaluation.getErrorMessage()); + providerName, evaluation.getErrorCode() + ": " + evaluation.getErrorMessage()); } } catch (Exception e) { - providerErrors.put( - providerName, - e.getClass().getSimpleName() + ": " - + e.getMessage()); + providerErrors.put(providerName, e.getClass().getSimpleName() + ": " + e.getMessage()); } return null; }); } - List> futures = - executorService.invokeAll( - tasks, timeoutMs, TimeUnit.MILLISECONDS); + List> futures = executorService.invokeAll(tasks, timeoutMs, TimeUnit.MILLISECONDS); for (Future future : futures) { if (future.isCancelled()) { return ProviderEvaluation.builder() .errorCode(ErrorCode.GENERAL) - .errorMessage( - "Comparison strategy timed out after " - + timeoutMs + "ms") + .errorMessage("Comparison strategy timed out after " + timeoutMs + "ms") .build(); } future.get(); } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return ProviderEvaluation.builder() + .errorCode(ErrorCode.GENERAL) + .errorMessage("Comparison strategy interrupted: " + e.getMessage()) + .build(); } catch (Exception e) { return ProviderEvaluation.builder() .errorCode(ErrorCode.GENERAL) - .errorMessage("Comparison strategy failed: " - + e.getMessage()) + .errorMessage("Comparison strategy failed: " + e.getMessage()) .build(); } if (!providerErrors.isEmpty()) { return ProviderEvaluation.builder() .errorCode(ErrorCode.GENERAL) - .errorMessage("Provider errors: " - + buildErrorSummary(providerErrors)) + .errorMessage("Provider errors: " + buildErrorSummary(providerErrors)) .build(); } - ProviderEvaluation fallbackResult = - successfulResults.get(fallbackProvider); + ProviderEvaluation fallbackResult = successfulResults.get(fallbackProvider); if (fallbackResult == null) { return ProviderEvaluation.builder() .errorCode(ErrorCode.GENERAL) - .errorMessage( - "Fallback provider did not return a successful " - + "evaluation: " + fallbackProvider) + .errorMessage("Fallback provider did not return a successful " + "evaluation: " + fallbackProvider) .build(); } @@ -207,10 +175,8 @@ public ProviderEvaluation evaluate( } if (onMismatch != null) { - Map> mismatchPayload = - new LinkedHashMap<>(successfulResults); - onMismatch.accept( - key, Collections.unmodifiableMap(mismatchPayload)); + Map> mismatchPayload = new LinkedHashMap<>(successfulResults); + onMismatch.accept(key, Collections.unmodifiableMap(mismatchPayload)); } return fallbackResult; } @@ -223,23 +189,19 @@ private String buildErrorSummary(Map providerErrors) { builder.append("; "); } first = false; - builder.append(entry.getKey()) - .append(" -> ") - .append(entry.getValue()); + builder.append(entry.getKey()).append(" -> ").append(entry.getValue()); } return builder.toString(); } - private boolean allEvaluationsMatch( - Map> results) { + private boolean allEvaluationsMatch(Map> results) { ProviderEvaluation baseline = null; for (ProviderEvaluation evaluation : results.values()) { if (baseline == null) { baseline = evaluation; continue; } - if (!Objects.equals( - baseline.getValue(), evaluation.getValue())) { + if (!Objects.equals(baseline.getValue(), evaluation.getValue())) { return false; } } diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java index b5b8822df..3bd467b61 100644 --- a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java +++ b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java @@ -63,8 +63,7 @@ public class MultiProvider extends EventProvider { private final Map providerStates = new ConcurrentHashMap<>(); private final Map> providerEventObservers = new ConcurrentHashMap<>(); - private final ThreadLocal hookExecutionContextThreadLocal = - new ThreadLocal<>(); + private final ThreadLocal hookExecutionContextThreadLocal = new ThreadLocal<>(); private final ClientMetadata hookClientMetadata = MultiProvider::getNAME; private final Map emptyHookHints = Collections.emptyMap(); private ProviderState aggregateState; @@ -110,15 +109,12 @@ public List getProviderHooks() { @Override public Optional before(HookContext ctx, Map hints) { hookExecutionContextThreadLocal.set( - new HookExecutionContext( - ctx.getClientMetadata(), - hints != null ? hints : emptyHookHints)); + new HookExecutionContext(ctx.getClientMetadata(), hints != null ? hints : emptyHookHints)); return Optional.empty(); } @Override - public void finallyAfter(HookContext ctx, FlagEvaluationDetails details, - Map hints) { + public void finallyAfter(HookContext ctx, FlagEvaluationDetails details, Map hints) { hookExecutionContextThreadLocal.remove(); } }; @@ -143,16 +139,16 @@ protected static Map buildProviders(List providersMap, - Map suffixesByBaseName) { + String baseName, Map providersMap, Map suffixesByBaseName) { if (!providersMap.containsKey(baseName)) { suffixesByBaseName.putIfAbsent(baseName, 1); return baseName; @@ -189,7 +185,8 @@ public void initialize(EvaluationContext evaluationContext) throws Exception { Map providersMetadata = new LinkedHashMap<>(); initializeProviderStates(); synchronized (this) { - emitAggregateStateChange(determineAggregateState(), ProviderEventDetails.builder().build()); + emitAggregateStateChange( + determineAggregateState(), ProviderEventDetails.builder().build()); } if (providers.isEmpty()) { @@ -370,7 +367,8 @@ public void shutdown() { } synchronized (this) { initializeProviderStates(); - emitAggregateStateChange(ProviderState.NOT_READY, ProviderEventDetails.builder().build()); + emitAggregateStateChange( + ProviderState.NOT_READY, ProviderEventDetails.builder().build()); } log.debug("shutdown end"); super.shutdown(); @@ -406,9 +404,7 @@ private void onChildProviderEvent(String providerName, ProviderEvent event, Prov } private synchronized void setProviderState( - String providerName, - ProviderState providerState, - ProviderEventDetails details) { + String providerName, ProviderState providerState, ProviderEventDetails details) { providerStates.put(providerName, providerState); ProviderState aggregate = determineAggregateState(); emitAggregateStateChange(aggregate, details); @@ -565,8 +561,7 @@ private Exception toEvaluationException(ProviderEvaluation providerEvaluation return new RuntimeException("Provider evaluation returned an error"); } return ExceptionUtils.instantiateErrorByErrorCode( - providerEvaluation.getErrorCode(), - providerEvaluation.getErrorMessage()); + providerEvaluation.getErrorCode(), providerEvaluation.getErrorMessage()); } @SuppressWarnings("deprecation") @@ -657,12 +652,16 @@ private ProviderEvaluation evaluateWithProviderHooks( for (int i = hooks.size() - 1; i >= 0; i--) { HookExecution execution = hooks.get(i); HookContext hookContext = createHookContext( - key, valueType, defaultValue, - evaluatedContext, provider, - hookExecutionContext, execution.hookData); + key, + valueType, + defaultValue, + evaluatedContext, + provider, + hookExecutionContext, + execution.hookData); var contextUpdate = execution.hook.before(hookContext, hookHints); if (contextUpdate != null - && contextUpdate.isPresent() + && contextUpdate.isPresent() // NOSONAR: hooks may return null instead of Optional.empty() && contextUpdate.get() != hookContext.getCtx() && !contextUpdate.get().isEmpty()) { evaluatedContext = evaluatedContext.merge(contextUpdate.get()); @@ -731,7 +730,10 @@ private ProviderEvaluation evaluateWithProviderHooks( throw e; } finally { FlagEvaluationDetails finalDetails = details == null - ? FlagEvaluationDetails.builder().flagKey(key).value(defaultValue).build() + ? FlagEvaluationDetails.builder() + .flagKey(key) + .value(defaultValue) + .build() : details; for (HookExecution execution : hooks) { try { diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java index 72628dfde..c726284e3 100644 --- a/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java +++ b/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java @@ -33,11 +33,7 @@ void shouldReturnFallbackResultWhenAllProvidersAgree() { ComparisonStrategy strategy = new ComparisonStrategy("provider2"); ProviderEvaluation result = strategy.evaluate( - providers, - FLAG_KEY, - DEFAULT_STRING, - null, - p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); + providers, FLAG_KEY, DEFAULT_STRING, null, p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); assertNotNull(result); assertEquals("same", result.getValue()); @@ -54,16 +50,11 @@ void shouldCallMismatchCallbackAndReturnFallbackResult() { providers.put("provider2", mockProvider2); AtomicInteger callbackCount = new AtomicInteger(); - ComparisonStrategy strategy = new ComparisonStrategy( - "provider2", - (key, evaluations) -> callbackCount.incrementAndGet()); + ComparisonStrategy strategy = + new ComparisonStrategy("provider2", (key, evaluations) -> callbackCount.incrementAndGet()); ProviderEvaluation result = strategy.evaluate( - providers, - FLAG_KEY, - DEFAULT_STRING, - null, - p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); + providers, FLAG_KEY, DEFAULT_STRING, null, p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); assertEquals("second", result.getValue()); assertNull(result.getErrorCode()); @@ -81,11 +72,7 @@ void shouldReturnGeneralErrorWhenAnyProviderFails() { ComparisonStrategy strategy = new ComparisonStrategy("provider1"); ProviderEvaluation result = strategy.evaluate( - providers, - FLAG_KEY, - DEFAULT_STRING, - null, - p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); + providers, FLAG_KEY, DEFAULT_STRING, null, p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); assertEquals(ErrorCode.GENERAL, result.getErrorCode()); assertTrue(result.getErrorMessage().contains("provider2")); @@ -126,24 +113,18 @@ void shouldEvaluateProvidersConcurrently() throws InterruptedException { setupProviderSuccess(mockProvider2, "val"); ComparisonStrategy strategy = new ComparisonStrategy("provider1"); - ProviderEvaluation result = strategy.evaluate( - providers, - FLAG_KEY, - DEFAULT_STRING, - null, - provider -> { - threadNames.add(Thread.currentThread().getName()); - bothStarted.countDown(); - try { - // Wait for both providers to signal they've started - bothStarted.await(5, TimeUnit.SECONDS); - proceed.countDown(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - return provider.getStringEvaluation( - FLAG_KEY, DEFAULT_STRING, null); - }); + ProviderEvaluation result = strategy.evaluate(providers, FLAG_KEY, DEFAULT_STRING, null, provider -> { + threadNames.add(Thread.currentThread().getName()); + bothStarted.countDown(); + try { + // Wait for both providers to signal they've started + bothStarted.await(5, TimeUnit.SECONDS); + proceed.countDown(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + return provider.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null); + }); assertNotNull(result); assertEquals("val", result.getValue()); @@ -151,8 +132,7 @@ void shouldEvaluateProvidersConcurrently() throws InterruptedException { // Verify that at least 2 different threads were used assertTrue( threadNames.size() >= 2, - "Expected concurrent execution on multiple threads, " - + "but only saw: " + threadNames); + "Expected concurrent execution on multiple threads, " + "but only saw: " + threadNames); } @Test @@ -166,8 +146,7 @@ void shouldReuseProvidedExecutorService() { providers.put("provider1", mockProvider1); providers.put("provider2", mockProvider2); - ComparisonStrategy strategy = new ComparisonStrategy( - "provider1", null, customExecutor, 5000); + ComparisonStrategy strategy = new ComparisonStrategy("provider1", null, customExecutor, 5000); // Execute twice to confirm the same executor is reused for (int i = 0; i < 2; i++) { @@ -176,17 +155,14 @@ void shouldReuseProvidedExecutorService() { FLAG_KEY, DEFAULT_STRING, null, - p -> p.getStringEvaluation( - FLAG_KEY, DEFAULT_STRING, null)); + p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); assertNotNull(result); assertEquals("ok", result.getValue()); assertNull(result.getErrorCode()); } // Executor should still be usable (not shut down) - assertTrue( - !customExecutor.isShutdown(), - "Executor should not be shut down by the strategy"); + assertTrue(!customExecutor.isShutdown(), "Executor should not be shut down by the strategy"); } finally { customExecutor.shutdown(); } @@ -203,18 +179,10 @@ void shouldCollectAllProviderErrorsWhenMultipleFail() { ComparisonStrategy strategy = new ComparisonStrategy("provider1"); ProviderEvaluation result = strategy.evaluate( - providers, - FLAG_KEY, - DEFAULT_STRING, - null, - p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); + providers, FLAG_KEY, DEFAULT_STRING, null, p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); assertEquals(ErrorCode.GENERAL, result.getErrorCode()); - assertTrue( - result.getErrorMessage().contains("provider1"), - "Error should mention provider1"); - assertTrue( - result.getErrorMessage().contains("provider2"), - "Error should mention provider2"); + assertTrue(result.getErrorMessage().contains("provider1"), "Error should mention provider1"); + assertTrue(result.getErrorMessage().contains("provider2"), "Error should mention provider2"); } } diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java index aed14ed49..06c00fee5 100644 --- a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java +++ b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderEventsAndTrackingTest.java @@ -39,14 +39,22 @@ void shouldAggregateChildProviderStateAndForwardConfigurationEvents() throws Exc AtomicInteger configurationChangedCount = new AtomicInteger(); client.onProviderConfigurationChanged(details -> configurationChangedCount.incrementAndGet()); - provider1.emitProviderConfigurationChanged(ProviderEventDetails.builder().message("changed").build()).await(); + provider1 + .emitProviderConfigurationChanged( + ProviderEventDetails.builder().message("changed").build()) + .await(); await().atMost(Duration.ofSeconds(2)).until(() -> configurationChangedCount.get() == 1); - provider1.emitProviderStale(ProviderEventDetails.builder().message("stale").build()).await(); + provider1 + .emitProviderStale( + ProviderEventDetails.builder().message("stale").build()) + .await(); await().atMost(Duration.ofSeconds(2)).until(() -> client.getProviderState() == ProviderState.STALE); - provider2.emitProviderError( - ProviderEventDetails.builder().errorCode(dev.openfeature.sdk.ErrorCode.GENERAL).build()) + provider2 + .emitProviderError(ProviderEventDetails.builder() + .errorCode(dev.openfeature.sdk.ErrorCode.GENERAL) + .build()) .await(); await().atMost(Duration.ofSeconds(2)).until(() -> client.getProviderState() == ProviderState.ERROR); @@ -56,10 +64,10 @@ void shouldAggregateChildProviderStateAndForwardConfigurationEvents() throws Exc provider1.emitProviderReady(ProviderEventDetails.builder().build()).await(); await().atMost(Duration.ofSeconds(2)).until(() -> client.getProviderState() == ProviderState.READY); - provider1.emitProviderError( - ProviderEventDetails.builder() - .errorCode(dev.openfeature.sdk.ErrorCode.PROVIDER_FATAL) - .build()) + provider1 + .emitProviderError(ProviderEventDetails.builder() + .errorCode(dev.openfeature.sdk.ErrorCode.PROVIDER_FATAL) + .build()) .await(); await().atMost(Duration.ofSeconds(2)).until(() -> client.getProviderState() == ProviderState.FATAL); } finally { @@ -94,10 +102,10 @@ void shouldForwardTrackToReadyProvidersAndSkipFatalProviders() throws Exception assertEquals(1, provider1.trackCount.get()); assertEquals(1, provider2.trackCount.get()); - provider1.emitProviderError( - ProviderEventDetails.builder() - .errorCode(dev.openfeature.sdk.ErrorCode.PROVIDER_FATAL) - .build()) + provider1 + .emitProviderError(ProviderEventDetails.builder() + .errorCode(dev.openfeature.sdk.ErrorCode.PROVIDER_FATAL) + .build()) .await(); multiProvider.track("event2", null, null); @@ -128,7 +136,8 @@ public void track(String eventName, EvaluationContext context, TrackingEventDeta } @Override - public ProviderEvaluation getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) { + public ProviderEvaluation getBooleanEvaluation( + String key, Boolean defaultValue, EvaluationContext ctx) { return ProviderEvaluation.builder().value(Boolean.TRUE).build(); } @@ -138,7 +147,8 @@ public ProviderEvaluation getStringEvaluation(String key, String default } @Override - public ProviderEvaluation getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx) { + public ProviderEvaluation getIntegerEvaluation( + String key, Integer defaultValue, EvaluationContext ctx) { return ProviderEvaluation.builder().value(1).build(); } @@ -164,7 +174,10 @@ static class InitializingStateProvider extends TrackingProvider { @Override public void initialize(EvaluationContext evaluationContext) throws Exception { if (ProviderState.STALE.equals(initializeState)) { - emitProviderStale(ProviderEventDetails.builder().message("stale during init").build()).await(); + emitProviderStale(ProviderEventDetails.builder() + .message("stale during init") + .build()) + .await(); } else if (ProviderState.FATAL.equals(initializeState)) { emitProviderError(ProviderEventDetails.builder() .errorCode(dev.openfeature.sdk.ErrorCode.PROVIDER_FATAL) diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java index dd4af41e4..c2b15bd5f 100644 --- a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java +++ b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java @@ -61,7 +61,9 @@ void shouldExecuteProviderHooksAndKeepPerProviderContextIsolation() throws Excep assertEquals(0, secondHook.errorCount.get()); assertEquals(1, secondHook.finallyCount.get()); - assertEquals("provider1", provider1.lastEvaluationContext.getValue("hookOwner").asString()); + assertEquals( + "provider1", + provider1.lastEvaluationContext.getValue("hookOwner").asString()); assertNull(provider1.lastEvaluationContext.getValue("provider2Marker")); assertNotNull(firstHook.lastFinallyDetails); assertEquals(ErrorCode.GENERAL, firstHook.lastFinallyDetails.getErrorCode()); @@ -69,7 +71,9 @@ void shouldExecuteProviderHooksAndKeepPerProviderContextIsolation() throws Excep assertEquals("default", firstHook.lastFinallyDetails.getValue()); assertEquals("failed", firstHook.lastFinallyDetails.getErrorMessage()); - assertEquals("provider2", provider2.lastEvaluationContext.getValue("hookOwner").asString()); + assertEquals( + "provider2", + provider2.lastEvaluationContext.getValue("hookOwner").asString()); assertNull(provider2.lastEvaluationContext.getValue("provider1Marker")); } @@ -80,8 +84,7 @@ void shouldExposeContextCapturingProviderHook() throws Exception { List.of(), ProviderEvaluation.builder().value("ok").build()); - MultiProvider multiProvider = new MultiProvider( - List.of(provider1), new FirstSuccessfulStrategy()); + MultiProvider multiProvider = new MultiProvider(List.of(provider1), new FirstSuccessfulStrategy()); multiProvider.initialize(null); // MultiProvider should expose a provider hook for context capture @@ -95,7 +98,8 @@ void shouldPassHookHintsAndClientMetadataAndEnrichThrownProviderErrors() throws RecordingHook firstHook = new RecordingHook("provider1"); RecordingHook secondHook = new RecordingHook("provider2"); - HookedStringProvider provider1 = new HookedStringProvider("provider1", List.of(firstHook), new RuntimeException("boom")); + HookedStringProvider provider1 = + new HookedStringProvider("provider1", List.of(firstHook), new RuntimeException("boom")); HookedStringProvider provider2 = new HookedStringProvider( "provider2", List.of(secondHook), @@ -113,7 +117,9 @@ void shouldPassHookHintsAndClientMetadataAndEnrichThrownProviderErrors() throws "flag", "default", new ImmutableContext(), - FlagEvaluationOptions.builder().hookHints(Map.of("hintKey", "hintValue")).build()); + FlagEvaluationOptions.builder() + .hookHints(Map.of("hintKey", "hintValue")) + .build()); assertEquals("ok", evaluation.getValue()); @@ -152,9 +158,8 @@ public Optional before(HookContext ctx, Map getProviderHooks() { } @Override - public ProviderEvaluation getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) { + public ProviderEvaluation getBooleanEvaluation( + String key, Boolean defaultValue, EvaluationContext ctx) { return ProviderEvaluation.builder().value(defaultValue).build(); } @@ -236,7 +242,8 @@ public ProviderEvaluation getStringEvaluation(String key, String default } @Override - public ProviderEvaluation getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx) { + public ProviderEvaluation getIntegerEvaluation( + String key, Integer defaultValue, EvaluationContext ctx) { return ProviderEvaluation.builder().value(defaultValue).build(); } From 586f4c64b968758c4fbb44dac68c3096e565801f Mon Sep 17 00:00:00 2001 From: Jonathan Norris Date: Fri, 8 May 2026 14:36:58 -0400 Subject: [PATCH 5/8] refactor: simplify multiprovider code per review Signed-off-by: Jonathan Norris --- .../sdk/multiprovider/ComparisonStrategy.java | 23 ++++------ .../sdk/multiprovider/MultiProvider.java | 42 +++++++++---------- 2 files changed, 29 insertions(+), 36 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java b/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java index 672dd7a82..eb95e4ade 100644 --- a/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java +++ b/src/main/java/dev/openfeature/sdk/multiprovider/ComparisonStrategy.java @@ -18,6 +18,7 @@ import java.util.concurrent.TimeUnit; import java.util.function.BiConsumer; import java.util.function.Function; +import java.util.stream.Collectors; import lombok.Getter; /** @@ -107,8 +108,9 @@ public ProviderEvaluation evaluate( throw new IllegalArgumentException("fallbackProvider not found in providers: " + fallbackProvider); } - Map> successfulResults = new ConcurrentHashMap<>(providers.size()); - Map providerErrors = new ConcurrentHashMap<>(providers.size()); + int capacity = providers.size() * 4 / 3 + 1; + Map> successfulResults = new ConcurrentHashMap<>(capacity); + Map providerErrors = new ConcurrentHashMap<>(capacity); try { List> tasks = new ArrayList<>(providers.size()); @@ -166,7 +168,7 @@ public ProviderEvaluation evaluate( if (fallbackResult == null) { return ProviderEvaluation.builder() .errorCode(ErrorCode.GENERAL) - .errorMessage("Fallback provider did not return a successful " + "evaluation: " + fallbackProvider) + .errorMessage("Fallback provider did not return a successful evaluation: " + fallbackProvider) .build(); } @@ -181,17 +183,10 @@ public ProviderEvaluation evaluate( return fallbackResult; } - private String buildErrorSummary(Map providerErrors) { - StringBuilder builder = new StringBuilder(); - boolean first = true; - for (Map.Entry entry : providerErrors.entrySet()) { - if (!first) { - builder.append("; "); - } - first = false; - builder.append(entry.getKey()).append(" -> ").append(entry.getValue()); - } - return builder.toString(); + private String buildErrorSummary(Map errors) { + return errors.entrySet().stream() + .map(e -> e.getKey() + " -> " + e.getValue()) + .collect(Collectors.joining("; ")); } private boolean allEvaluationsMatch(Map> results) { diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java index 3bd467b61..bc142d3cc 100644 --- a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java +++ b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java @@ -65,7 +65,6 @@ public class MultiProvider extends EventProvider { new ConcurrentHashMap<>(); private final ThreadLocal hookExecutionContextThreadLocal = new ThreadLocal<>(); private final ClientMetadata hookClientMetadata = MultiProvider::getNAME; - private final Map emptyHookHints = Collections.emptyMap(); private ProviderState aggregateState; private MultiProviderMetadata metadata; @@ -103,22 +102,23 @@ public MultiProvider(List providers, Strategy strategy) { * @return the list of provider hooks */ @SuppressWarnings({"rawtypes", "unchecked"}) + private final List providerHooks = List.of(new Hook() { + @Override + public Optional before(HookContext ctx, Map hints) { + hookExecutionContextThreadLocal.set( + new HookExecutionContext(ctx.getClientMetadata(), hints != null ? hints : Collections.emptyMap())); + return Optional.empty(); + } + + @Override + public void finallyAfter(HookContext ctx, FlagEvaluationDetails details, Map hints) { + hookExecutionContextThreadLocal.remove(); + } + }); + @Override public List getProviderHooks() { - Hook contextCapturingHook = new Hook() { - @Override - public Optional before(HookContext ctx, Map hints) { - hookExecutionContextThreadLocal.set( - new HookExecutionContext(ctx.getClientMetadata(), hints != null ? hints : emptyHookHints)); - return Optional.empty(); - } - - @Override - public void finallyAfter(HookContext ctx, FlagEvaluationDetails details, Map hints) { - hookExecutionContextThreadLocal.remove(); - } - }; - return List.of(contextCapturingHook); + return providerHooks; } protected static Map buildProviders(List providers) { @@ -220,13 +220,10 @@ public void initialize(EvaluationContext evaluationContext) throws Exception { List> results = executorService.invokeAll(tasks); for (Future result : results) { - // This will re-throw any exception from the provider's initialize method, - // wrapped in an ExecutionException. result.get(); } } catch (Exception e) { - // If initialization fails for any provider, attempt to shut down via the - // standard shutdown path to avoid a partial/limbo state. + // Clean up to avoid leaving child providers in a partially initialised state. try { shutdown(); } catch (Exception shutdownEx) { @@ -660,8 +657,9 @@ private ProviderEvaluation evaluateWithProviderHooks( hookExecutionContext, execution.hookData); var contextUpdate = execution.hook.before(hookContext, hookHints); - if (contextUpdate != null - && contextUpdate.isPresent() // NOSONAR: hooks may return null instead of Optional.empty() + // Raw-type invocation: third-party hooks predating Optional may return null. + if (contextUpdate != null // NOSONAR + && contextUpdate.isPresent() && contextUpdate.get() != hookContext.getCtx() && !contextUpdate.get().isEmpty()) { evaluatedContext = evaluatedContext.merge(contextUpdate.get()); @@ -768,7 +766,7 @@ private ClientMetadata resolveClientMetadata(HookExecutionContext hookExecutionC private Map resolveHookHints(HookExecutionContext hookExecutionContext) { if (hookExecutionContext == null || hookExecutionContext.hints == null) { - return emptyHookHints; + return Collections.emptyMap(); } return hookExecutionContext.hints; } From 7e498ef2d31197f5a5820577839458c69c369de3 Mon Sep 17 00:00:00 2001 From: Jonathan Norris Date: Fri, 8 May 2026 14:43:54 -0400 Subject: [PATCH 6/8] refactor: extract MultiProviderHookExecutor and HookExecutionContext from MultiProvider Signed-off-by: Jonathan Norris --- .../multiprovider/HookExecutionContext.java | 15 + .../sdk/multiprovider/MultiProvider.java | 289 +---------------- .../MultiProviderHookExecutor.java | 302 ++++++++++++++++++ 3 files changed, 323 insertions(+), 283 deletions(-) create mode 100644 src/main/java/dev/openfeature/sdk/multiprovider/HookExecutionContext.java create mode 100644 src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutor.java diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/HookExecutionContext.java b/src/main/java/dev/openfeature/sdk/multiprovider/HookExecutionContext.java new file mode 100644 index 000000000..5b0d501d9 --- /dev/null +++ b/src/main/java/dev/openfeature/sdk/multiprovider/HookExecutionContext.java @@ -0,0 +1,15 @@ +package dev.openfeature.sdk.multiprovider; + +import dev.openfeature.sdk.ClientMetadata; +import java.util.Map; + +/** Captures hook lifecycle context (client metadata and hints) for per-provider hook execution. */ +final class HookExecutionContext { + final ClientMetadata clientMetadata; + final Map hints; + + HookExecutionContext(ClientMetadata clientMetadata, Map hints) { + this.clientMetadata = clientMetadata; + this.hints = hints; + } +} diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java index bc142d3cc..ad11b9dcd 100644 --- a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java +++ b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java @@ -1,7 +1,6 @@ package dev.openfeature.sdk.multiprovider; import dev.openfeature.sdk.ClientMetadata; -import dev.openfeature.sdk.DefaultHookData; import dev.openfeature.sdk.ErrorCode; import dev.openfeature.sdk.EvaluationContext; import dev.openfeature.sdk.EventProvider; @@ -10,17 +9,13 @@ import dev.openfeature.sdk.FlagValueType; import dev.openfeature.sdk.Hook; import dev.openfeature.sdk.HookContext; -import dev.openfeature.sdk.HookData; -import dev.openfeature.sdk.ImmutableContext; import dev.openfeature.sdk.Metadata; import dev.openfeature.sdk.ProviderEvaluation; import dev.openfeature.sdk.ProviderEvent; import dev.openfeature.sdk.ProviderEventDetails; import dev.openfeature.sdk.ProviderState; -import dev.openfeature.sdk.Reason; import dev.openfeature.sdk.TrackingEventDetails; import dev.openfeature.sdk.Value; -import dev.openfeature.sdk.exceptions.ExceptionUtils; import dev.openfeature.sdk.exceptions.OpenFeatureError; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.ArrayList; @@ -38,7 +33,6 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.function.BiConsumer; -import java.util.function.BiFunction; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -65,6 +59,7 @@ public class MultiProvider extends EventProvider { new ConcurrentHashMap<>(); private final ThreadLocal hookExecutionContextThreadLocal = new ThreadLocal<>(); private final ClientMetadata hookClientMetadata = MultiProvider::getNAME; + private final MultiProviderHookExecutor hookExecutor = new MultiProviderHookExecutor(hookClientMetadata); private ProviderState aggregateState; private MultiProviderMetadata metadata; @@ -251,7 +246,7 @@ public ProviderEvaluation getBooleanEvaluation(String key, Boolean defa key, defaultValue, ctx, - provider -> evaluateWithProviderHooks( + provider -> hookExecutor.evaluate( provider, key, defaultValue, @@ -269,7 +264,7 @@ public ProviderEvaluation getStringEvaluation(String key, String default key, defaultValue, ctx, - provider -> evaluateWithProviderHooks( + provider -> hookExecutor.evaluate( provider, key, defaultValue, @@ -287,7 +282,7 @@ public ProviderEvaluation getIntegerEvaluation(String key, Integer defa key, defaultValue, ctx, - provider -> evaluateWithProviderHooks( + provider -> hookExecutor.evaluate( provider, key, defaultValue, @@ -305,7 +300,7 @@ public ProviderEvaluation getDoubleEvaluation(String key, Double default key, defaultValue, ctx, - provider -> evaluateWithProviderHooks( + provider -> hookExecutor.evaluate( provider, key, defaultValue, @@ -323,7 +318,7 @@ public ProviderEvaluation getObjectEvaluation(String key, Value defaultVa key, defaultValue, ctx, - provider -> evaluateWithProviderHooks( + provider -> hookExecutor.evaluate( provider, key, defaultValue, @@ -535,279 +530,7 @@ private boolean shouldTrackProvider(String providerName) { return !ProviderState.NOT_READY.equals(providerState) && !ProviderState.FATAL.equals(providerState); } - private EvaluationContext copyEvaluationContext(EvaluationContext context) { - if (context == null) { - return ImmutableContext.EMPTY; - } - String targetingKey = context.getTargetingKey(); - if (targetingKey == null) { - return new ImmutableContext(context.asMap()); - } - return new ImmutableContext(targetingKey, context.asMap()); - } - - private EvaluationContext toProviderContext(EvaluationContext originalContext, EvaluationContext evaluatedContext) { - if (originalContext == null && (evaluatedContext == null || evaluatedContext.isEmpty())) { - return null; - } - return evaluatedContext; - } - - private Exception toEvaluationException(ProviderEvaluation providerEvaluation) { - if (providerEvaluation == null || providerEvaluation.getErrorCode() == null) { - return new RuntimeException("Provider evaluation returned an error"); - } - return ExceptionUtils.instantiateErrorByErrorCode( - providerEvaluation.getErrorCode(), providerEvaluation.getErrorMessage()); - } - - @SuppressWarnings("deprecation") - private HookContext createHookContext( - String key, - FlagValueType valueType, - T defaultValue, - EvaluationContext evaluationContext, - FeatureProvider provider, - HookExecutionContext hookExecutionContext, - HookData hookData) { - return HookContext.builder() - .flagKey(key) - .type(valueType) - .defaultValue(normalizeDefaultValue(valueType, defaultValue)) - .ctx(evaluationContext) - .clientMetadata(resolveClientMetadata(hookExecutionContext)) - .providerMetadata(provider.getMetadata()) - .hookData(hookData) - .build(); - } - - /** - * Returns a non-null default value for use in hook contexts when the caller - * passes {@code null}. The returned object is always assignment-compatible - * with the expected type for {@code valueType}. - */ - @SuppressWarnings("unchecked") - private T normalizeDefaultValue(FlagValueType valueType, T defaultValue) { - if (defaultValue != null) { - return defaultValue; - } - Object fallback; - switch (valueType) { - case BOOLEAN: - fallback = Boolean.FALSE; - break; - case STRING: - fallback = ""; - break; - case INTEGER: - fallback = Integer.valueOf(0); - break; - case DOUBLE: - fallback = Double.valueOf(0d); - break; - case OBJECT: - fallback = new Value(); - break; - default: - return defaultValue; - } - // Safe: the SDK guarantees T matches the valueType enum. - return (T) fallback; - } - - @SuppressWarnings({"rawtypes", "unchecked"}) - private ProviderEvaluation evaluateWithProviderHooks( - FeatureProvider provider, - String key, - T defaultValue, - EvaluationContext ctx, - HookExecutionContext hookExecutionContext, - FlagValueType valueType, - BiFunction> providerFunction) { - List providerHooks = provider.getProviderHooks(); - if (providerHooks == null || providerHooks.isEmpty()) { - return providerFunction.apply(provider, ctx); - } - - List> hooks = new ArrayList<>(providerHooks.size()); - for (Hook hook : providerHooks) { - if (hook.supportsFlagValueType(valueType)) { - hooks.add(new HookExecution<>(hook, new DefaultHookData())); - } - } - - if (hooks.isEmpty()) { - return providerFunction.apply(provider, ctx); - } - - EvaluationContext evaluatedContext = copyEvaluationContext(ctx); - ProviderEvaluation providerEvaluation = null; - FlagEvaluationDetails details = null; - Map hookHints = resolveHookHints(hookExecutionContext); - - try { - for (int i = hooks.size() - 1; i >= 0; i--) { - HookExecution execution = hooks.get(i); - HookContext hookContext = createHookContext( - key, - valueType, - defaultValue, - evaluatedContext, - provider, - hookExecutionContext, - execution.hookData); - var contextUpdate = execution.hook.before(hookContext, hookHints); - // Raw-type invocation: third-party hooks predating Optional may return null. - if (contextUpdate != null // NOSONAR - && contextUpdate.isPresent() - && contextUpdate.get() != hookContext.getCtx() - && !contextUpdate.get().isEmpty()) { - evaluatedContext = evaluatedContext.merge(contextUpdate.get()); - } - } - - providerEvaluation = providerFunction.apply(provider, toProviderContext(ctx, evaluatedContext)); - details = FlagEvaluationDetails.from(providerEvaluation, key); - - if (providerEvaluation.getErrorCode() == null) { - for (HookExecution execution : hooks) { - execution.hook.after( - createHookContext( - key, - valueType, - defaultValue, - evaluatedContext, - provider, - hookExecutionContext, - execution.hookData), - details, - hookHints); - } - } else { - enrichDetailsWithErrorDefaults(defaultValue, details); - Exception providerException = toEvaluationException(providerEvaluation); - for (HookExecution execution : hooks) { - try { - execution.hook.error( - createHookContext( - key, - valueType, - defaultValue, - evaluatedContext, - provider, - hookExecutionContext, - execution.hookData), - providerException, - hookHints); - } catch (Exception e) { - log.error("error executing provider hook error stage", e); - } - } - } - - return providerEvaluation; - } catch (Exception e) { - details = buildErrorDetails(key, defaultValue, details, e); - for (HookExecution execution : hooks) { - try { - execution.hook.error( - createHookContext( - key, - valueType, - defaultValue, - evaluatedContext, - provider, - hookExecutionContext, - execution.hookData), - e, - hookHints); - } catch (Exception hookError) { - log.error("error executing provider hook error stage", hookError); - } - } - throw e; - } finally { - FlagEvaluationDetails finalDetails = details == null - ? FlagEvaluationDetails.builder() - .flagKey(key) - .value(defaultValue) - .build() - : details; - for (HookExecution execution : hooks) { - try { - execution.hook.finallyAfter( - createHookContext( - key, - valueType, - defaultValue, - evaluatedContext, - provider, - hookExecutionContext, - execution.hookData), - finalDetails, - hookHints); - } catch (Exception e) { - log.error("error executing provider hook finally stage", e); - } - } - } - } - private HookExecutionContext currentHookExecutionContext() { return hookExecutionContextThreadLocal.get(); } - - private ClientMetadata resolveClientMetadata(HookExecutionContext hookExecutionContext) { - if (hookExecutionContext == null || hookExecutionContext.clientMetadata == null) { - return hookClientMetadata; - } - return hookExecutionContext.clientMetadata; - } - - private Map resolveHookHints(HookExecutionContext hookExecutionContext) { - if (hookExecutionContext == null || hookExecutionContext.hints == null) { - return Collections.emptyMap(); - } - return hookExecutionContext.hints; - } - - private FlagEvaluationDetails buildErrorDetails( - String key, T defaultValue, FlagEvaluationDetails details, Exception error) { - FlagEvaluationDetails errorDetails = details == null - ? FlagEvaluationDetails.builder().flagKey(key).build() - : details; - if (error instanceof OpenFeatureError) { - errorDetails.setErrorCode(((OpenFeatureError) error).getErrorCode()); - } else { - errorDetails.setErrorCode(ErrorCode.GENERAL); - } - errorDetails.setErrorMessage(error.getMessage()); - enrichDetailsWithErrorDefaults(defaultValue, errorDetails); - return errorDetails; - } - - private void enrichDetailsWithErrorDefaults(T defaultValue, FlagEvaluationDetails details) { - details.setValue(defaultValue); - details.setReason(Reason.ERROR.toString()); - } - - private static final class HookExecution { - private final Hook hook; - private final HookData hookData; - - private HookExecution(Hook hook, HookData hookData) { - this.hook = hook; - this.hookData = hookData; - } - } - - private static final class HookExecutionContext { - private final ClientMetadata clientMetadata; - private final Map hints; - - private HookExecutionContext(ClientMetadata clientMetadata, Map hints) { - this.clientMetadata = clientMetadata; - this.hints = hints; - } - } } diff --git a/src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutor.java b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutor.java new file mode 100644 index 000000000..336b6dc9f --- /dev/null +++ b/src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutor.java @@ -0,0 +1,302 @@ +package dev.openfeature.sdk.multiprovider; + +import dev.openfeature.sdk.ClientMetadata; +import dev.openfeature.sdk.DefaultHookData; +import dev.openfeature.sdk.ErrorCode; +import dev.openfeature.sdk.EvaluationContext; +import dev.openfeature.sdk.FeatureProvider; +import dev.openfeature.sdk.FlagEvaluationDetails; +import dev.openfeature.sdk.FlagValueType; +import dev.openfeature.sdk.Hook; +import dev.openfeature.sdk.HookContext; +import dev.openfeature.sdk.HookData; +import dev.openfeature.sdk.ImmutableContext; +import dev.openfeature.sdk.ProviderEvaluation; +import dev.openfeature.sdk.Reason; +import dev.openfeature.sdk.Value; +import dev.openfeature.sdk.exceptions.ExceptionUtils; +import dev.openfeature.sdk.exceptions.OpenFeatureError; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.function.BiFunction; +import lombok.extern.slf4j.Slf4j; + +/** + * Runs per-provider hook lifecycles during flag evaluation. + * + *

Mirrors the role of {@code HookExecutor} in the JS SDK: executes the before/after/error/finally + * stages for each child provider's own hooks, using context captured by {@link MultiProvider}'s + * provider-level hook. + */ +@Slf4j +class MultiProviderHookExecutor { + + private final ClientMetadata fallbackClientMetadata; + + MultiProviderHookExecutor(ClientMetadata fallbackClientMetadata) { + this.fallbackClientMetadata = fallbackClientMetadata; + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + ProviderEvaluation evaluate( + FeatureProvider provider, + String key, + T defaultValue, + EvaluationContext ctx, + HookExecutionContext hookExecutionContext, + FlagValueType valueType, + BiFunction> providerFunction) { + List rawHooks = provider.getProviderHooks(); + if (rawHooks == null || rawHooks.isEmpty()) { + return providerFunction.apply(provider, ctx); + } + + List> hooks = new ArrayList<>(rawHooks.size()); + for (Hook hook : rawHooks) { + if (hook.supportsFlagValueType(valueType)) { + hooks.add(new HookExecution<>(hook, new DefaultHookData())); + } + } + + if (hooks.isEmpty()) { + return providerFunction.apply(provider, ctx); + } + + EvaluationContext evaluatedContext = copyEvaluationContext(ctx); + ProviderEvaluation providerEvaluation = null; + FlagEvaluationDetails details = null; + Map hookHints = resolveHookHints(hookExecutionContext); + + try { + for (int i = hooks.size() - 1; i >= 0; i--) { + HookExecution execution = hooks.get(i); + HookContext hookContext = createHookContext( + key, + valueType, + defaultValue, + evaluatedContext, + provider, + hookExecutionContext, + execution.hookData); + var contextUpdate = execution.hook.before(hookContext, hookHints); + // Raw-type invocation: third-party hooks predating Optional may return null. + if (contextUpdate != null // NOSONAR + && contextUpdate.isPresent() + && contextUpdate.get() != hookContext.getCtx() + && !contextUpdate.get().isEmpty()) { + evaluatedContext = evaluatedContext.merge(contextUpdate.get()); + } + } + + providerEvaluation = providerFunction.apply(provider, toProviderContext(ctx, evaluatedContext)); + details = FlagEvaluationDetails.from(providerEvaluation, key); + + if (providerEvaluation.getErrorCode() == null) { + for (HookExecution execution : hooks) { + execution.hook.after( + createHookContext( + key, + valueType, + defaultValue, + evaluatedContext, + provider, + hookExecutionContext, + execution.hookData), + details, + hookHints); + } + } else { + enrichDetailsWithErrorDefaults(defaultValue, details); + Exception providerException = toEvaluationException(providerEvaluation); + for (HookExecution execution : hooks) { + try { + execution.hook.error( + createHookContext( + key, + valueType, + defaultValue, + evaluatedContext, + provider, + hookExecutionContext, + execution.hookData), + providerException, + hookHints); + } catch (Exception e) { + log.error("error executing provider hook error stage", e); + } + } + } + + return providerEvaluation; + } catch (Exception e) { + details = buildErrorDetails(key, defaultValue, details, e); + for (HookExecution execution : hooks) { + try { + execution.hook.error( + createHookContext( + key, + valueType, + defaultValue, + evaluatedContext, + provider, + hookExecutionContext, + execution.hookData), + e, + hookHints); + } catch (Exception hookError) { + log.error("error executing provider hook error stage", hookError); + } + } + throw e; + } finally { + FlagEvaluationDetails finalDetails = details == null + ? FlagEvaluationDetails.builder() + .flagKey(key) + .value(defaultValue) + .build() + : details; + for (HookExecution execution : hooks) { + try { + execution.hook.finallyAfter( + createHookContext( + key, + valueType, + defaultValue, + evaluatedContext, + provider, + hookExecutionContext, + execution.hookData), + finalDetails, + hookHints); + } catch (Exception e) { + log.error("error executing provider hook finally stage", e); + } + } + } + } + + private EvaluationContext copyEvaluationContext(EvaluationContext context) { + if (context == null) { + return ImmutableContext.EMPTY; + } + String targetingKey = context.getTargetingKey(); + if (targetingKey == null) { + return new ImmutableContext(context.asMap()); + } + return new ImmutableContext(targetingKey, context.asMap()); + } + + private EvaluationContext toProviderContext(EvaluationContext originalContext, EvaluationContext evaluatedContext) { + if (originalContext == null && (evaluatedContext == null || evaluatedContext.isEmpty())) { + return null; + } + return evaluatedContext; + } + + private Exception toEvaluationException(ProviderEvaluation providerEvaluation) { + if (providerEvaluation == null || providerEvaluation.getErrorCode() == null) { + return new RuntimeException("Provider evaluation returned an error"); + } + return ExceptionUtils.instantiateErrorByErrorCode( + providerEvaluation.getErrorCode(), providerEvaluation.getErrorMessage()); + } + + @SuppressWarnings("deprecation") + private HookContext createHookContext( + String key, + FlagValueType valueType, + T defaultValue, + EvaluationContext evaluationContext, + FeatureProvider provider, + HookExecutionContext hookExecutionContext, + HookData hookData) { + return HookContext.builder() + .flagKey(key) + .type(valueType) + .defaultValue(normalizeDefaultValue(valueType, defaultValue)) + .ctx(evaluationContext) + .clientMetadata(resolveClientMetadata(hookExecutionContext)) + .providerMetadata(provider.getMetadata()) + .hookData(hookData) + .build(); + } + + /** + * Returns a non-null default value for use in hook contexts when the caller passes {@code null}. + * The returned object is always assignment-compatible with the expected type for {@code valueType}. + */ + @SuppressWarnings("unchecked") + private T normalizeDefaultValue(FlagValueType valueType, T defaultValue) { + if (defaultValue != null) { + return defaultValue; + } + Object fallback; + switch (valueType) { + case BOOLEAN: + fallback = Boolean.FALSE; + break; + case STRING: + fallback = ""; + break; + case INTEGER: + fallback = Integer.valueOf(0); + break; + case DOUBLE: + fallback = Double.valueOf(0d); + break; + case OBJECT: + fallback = new Value(); + break; + default: + return defaultValue; + } + // Safe: the SDK guarantees T matches the valueType enum. + return (T) fallback; + } + + private ClientMetadata resolveClientMetadata(HookExecutionContext hookExecutionContext) { + if (hookExecutionContext == null || hookExecutionContext.clientMetadata == null) { + return fallbackClientMetadata; + } + return hookExecutionContext.clientMetadata; + } + + private Map resolveHookHints(HookExecutionContext hookExecutionContext) { + if (hookExecutionContext == null || hookExecutionContext.hints == null) { + return Collections.emptyMap(); + } + return hookExecutionContext.hints; + } + + private FlagEvaluationDetails buildErrorDetails( + String key, T defaultValue, FlagEvaluationDetails details, Exception error) { + FlagEvaluationDetails errorDetails = details == null + ? FlagEvaluationDetails.builder().flagKey(key).build() + : details; + if (error instanceof OpenFeatureError) { + errorDetails.setErrorCode(((OpenFeatureError) error).getErrorCode()); + } else { + errorDetails.setErrorCode(ErrorCode.GENERAL); + } + errorDetails.setErrorMessage(error.getMessage()); + enrichDetailsWithErrorDefaults(defaultValue, errorDetails); + return errorDetails; + } + + private void enrichDetailsWithErrorDefaults(T defaultValue, FlagEvaluationDetails details) { + details.setValue(defaultValue); + details.setReason(Reason.ERROR.toString()); + } + + private static final class HookExecution { + private final Hook hook; + private final HookData hookData; + + private HookExecution(Hook hook, HookData hookData) { + this.hook = hook; + this.hookData = hookData; + } + } +} From 0ad3c118d1a6cea5f0e0b9315075a103cef98c19 Mon Sep 17 00:00:00 2001 From: Jonathan Norris Date: Fri, 8 May 2026 14:52:48 -0400 Subject: [PATCH 7/8] test: add unit tests for MultiProviderHookExecutor edge cases Signed-off-by: Jonathan Norris --- .../MultiProviderHookExecutorTest.java | 287 ++++++++++++++++++ 1 file changed, 287 insertions(+) create mode 100644 src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutorTest.java diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutorTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutorTest.java new file mode 100644 index 000000000..710127119 --- /dev/null +++ b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHookExecutorTest.java @@ -0,0 +1,287 @@ +package dev.openfeature.sdk.multiprovider; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import dev.openfeature.sdk.EvaluationContext; +import dev.openfeature.sdk.EventProvider; +import dev.openfeature.sdk.FeatureProvider; +import dev.openfeature.sdk.FlagEvaluationDetails; +import dev.openfeature.sdk.FlagValueType; +import dev.openfeature.sdk.Hook; +import dev.openfeature.sdk.HookContext; +import dev.openfeature.sdk.Metadata; +import dev.openfeature.sdk.ProviderEvaluation; +import dev.openfeature.sdk.Value; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.Test; + +class MultiProviderHookExecutorTest { + + private final MultiProviderHookExecutor executor = new MultiProviderHookExecutor(() -> "test"); + + @Test + void shortCircuitsDirectlyWhenProviderHasNoHooks() { + AtomicBoolean called = new AtomicBoolean(false); + ProviderEvaluation result = executor.evaluate( + stubProvider("p", Collections.emptyList()), + "flag", + "default", + null, + null, + FlagValueType.STRING, + (p, ctx) -> { + called.set(true); + return ProviderEvaluation.builder().value("direct").build(); + }); + + assertTrue(called.get()); + assertEquals("direct", result.getValue()); + } + + @Test + void shortCircuitsWhenNoHooksSupportTheFlagType() { + AtomicBoolean called = new AtomicBoolean(false); + Hook boolOnlyHook = new Hook() { + @Override + public boolean supportsFlagValueType(FlagValueType type) { + return type == FlagValueType.BOOLEAN; + } + }; + ProviderEvaluation result = executor.evaluate( + stubProvider("p", List.of(boolOnlyHook)), + "flag", + "default", + null, + null, + FlagValueType.STRING, + (p, ctx) -> { + called.set(true); + return ProviderEvaluation.builder().value("direct").build(); + }); + + assertTrue(called.get()); + assertEquals("direct", result.getValue()); + } + + @Test + @SuppressWarnings("rawtypes") + void toleratesNullReturnedFromBeforeHook() { + Hook nullBeforeHook = new Hook() { + @Override + public Optional before(HookContext ctx, Map hints) { + return null; + } + }; + ProviderEvaluation result = executor.evaluate( + stubProvider("p", List.of(nullBeforeHook)), + "flag", + "default", + null, + null, + FlagValueType.STRING, + (p, ctx) -> ProviderEvaluation.builder().value("ok").build()); + + assertEquals("ok", result.getValue()); + } + + @Test + void swallowsExceptionThrownFromErrorHook() { + AtomicBoolean errorHookCalled = new AtomicBoolean(false); + Hook throwingErrorHook = new Hook() { + @Override + public void error(HookContext ctx, Exception error, Map hints) { + errorHookCalled.set(true); + throw new RuntimeException("error hook exploded"); + } + }; + RuntimeException providerEx = new RuntimeException("provider failed"); + + RuntimeException thrown = assertThrows( + RuntimeException.class, + () -> executor.evaluate( + stubProvider("p", List.of(throwingErrorHook)), + "flag", + "default", + null, + null, + FlagValueType.STRING, + (p, ctx) -> { + throw providerEx; + })); + + assertTrue(errorHookCalled.get(), "error() hook should have been called"); + assertEquals(providerEx, thrown, "original provider exception must propagate"); + } + + @Test + void swallowsExceptionThrownFromFinallyAfterHook() { + Hook throwingFinallyHook = new Hook() { + @Override + public void finallyAfter( + HookContext ctx, FlagEvaluationDetails details, Map hints) { + throw new RuntimeException("finallyAfter exploded"); + } + }; + + assertDoesNotThrow(() -> executor.evaluate( + stubProvider("p", List.of(throwingFinallyHook)), + "flag", + "default", + null, + null, + FlagValueType.STRING, + (p, ctx) -> ProviderEvaluation.builder().value("ok").build())); + } + + @Test + void finallyAfterReceivesSyntheticDetailsWhenBeforeThrows() { + AtomicReference> captured = new AtomicReference<>(); + Hook hook = new Hook() { + @Override + public Optional before(HookContext ctx, Map hints) { + throw new RuntimeException("before failed"); + } + + @Override + public void finallyAfter( + HookContext ctx, FlagEvaluationDetails details, Map hints) { + captured.set(details); + } + }; + + assertThrows( + RuntimeException.class, + () -> executor.evaluate( + stubProvider("p", List.of(hook)), + "flag", + "fallback", + null, + null, + FlagValueType.STRING, + (p, ctx) -> + ProviderEvaluation.builder().value("ok").build())); + + assertNotNull(captured.get(), "finallyAfter must be called even when before() throws"); + assertEquals("flag", captured.get().getFlagKey()); + assertEquals("fallback", captured.get().getValue()); + } + + @Test + @SuppressWarnings({"rawtypes", "unchecked"}) + void normalizesNullDefaultValueForEachFlagType() { + AtomicReference capturedDefault = new AtomicReference<>(); + Hook capturingHook = new Hook() { + @Override + public Optional before(HookContext ctx, Map hints) { + capturedDefault.set(ctx.getDefaultValue()); + return Optional.empty(); + } + }; + FeatureProvider provider = stubProvider("p", List.of(capturingHook)); + + executor.evaluate( + provider, + "f", + (Boolean) null, + null, + null, + FlagValueType.BOOLEAN, + (p, ctx) -> ProviderEvaluation.builder().value(false).build()); + assertEquals(Boolean.FALSE, capturedDefault.get()); + + executor.evaluate( + provider, + "f", + (String) null, + null, + null, + FlagValueType.STRING, + (p, ctx) -> ProviderEvaluation.builder().value("").build()); + assertEquals("", capturedDefault.get()); + + executor.evaluate( + provider, + "f", + (Integer) null, + null, + null, + FlagValueType.INTEGER, + (p, ctx) -> ProviderEvaluation.builder().value(0).build()); + assertEquals(0, capturedDefault.get()); + + executor.evaluate( + provider, + "f", + (Double) null, + null, + null, + FlagValueType.DOUBLE, + (p, ctx) -> ProviderEvaluation.builder().value(0d).build()); + assertEquals(0d, capturedDefault.get()); + + executor.evaluate( + provider, + "f", + (Value) null, + null, + null, + FlagValueType.OBJECT, + (p, ctx) -> + ProviderEvaluation.builder().value(new Value()).build()); + assertNotNull(capturedDefault.get()); + } + + @SuppressWarnings("rawtypes") + private static FeatureProvider stubProvider(String name, List hooks) { + return new EventProvider() { + @Override + public Metadata getMetadata() { + return () -> name; + } + + @Override + public List getProviderHooks() { + return hooks; + } + + @Override + public ProviderEvaluation getBooleanEvaluation( + String key, Boolean defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + + @Override + public ProviderEvaluation getStringEvaluation( + String key, String defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + + @Override + public ProviderEvaluation getIntegerEvaluation( + String key, Integer defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + + @Override + public ProviderEvaluation getDoubleEvaluation( + String key, Double defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + + @Override + public ProviderEvaluation getObjectEvaluation( + String key, Value defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder().value(defaultValue).build(); + } + }; + } +} From 29f2e01e6c4255df6b4ba27f0540b1862342ce55 Mon Sep 17 00:00:00 2001 From: Jonathan Norris Date: Fri, 8 May 2026 15:00:00 -0400 Subject: [PATCH 8/8] test: remove low-value tests Signed-off-by: Jonathan Norris --- .../multiprovider/ComparisonStrategyTest.java | 35 ------------------- .../multiprovider/MultiProviderHooksTest.java | 16 --------- 2 files changed, 51 deletions(-) diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java index c726284e3..e5c7df944 100644 --- a/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java +++ b/src/test/java/dev/openfeature/sdk/multiprovider/ComparisonStrategyTest.java @@ -14,8 +14,6 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Test; @@ -135,39 +133,6 @@ void shouldEvaluateProvidersConcurrently() throws InterruptedException { "Expected concurrent execution on multiple threads, " + "but only saw: " + threadNames); } - @Test - void shouldReuseProvidedExecutorService() { - ExecutorService customExecutor = Executors.newFixedThreadPool(2); - try { - setupProviderSuccess(mockProvider1, "ok"); - setupProviderSuccess(mockProvider2, "ok"); - - Map providers = new LinkedHashMap<>(); - providers.put("provider1", mockProvider1); - providers.put("provider2", mockProvider2); - - ComparisonStrategy strategy = new ComparisonStrategy("provider1", null, customExecutor, 5000); - - // Execute twice to confirm the same executor is reused - for (int i = 0; i < 2; i++) { - ProviderEvaluation result = strategy.evaluate( - providers, - FLAG_KEY, - DEFAULT_STRING, - null, - p -> p.getStringEvaluation(FLAG_KEY, DEFAULT_STRING, null)); - assertNotNull(result); - assertEquals("ok", result.getValue()); - assertNull(result.getErrorCode()); - } - - // Executor should still be usable (not shut down) - assertTrue(!customExecutor.isShutdown(), "Executor should not be shut down by the strategy"); - } finally { - customExecutor.shutdown(); - } - } - @Test void shouldCollectAllProviderErrorsWhenMultipleFail() { setupProviderError(mockProvider1, ErrorCode.PARSE_ERROR); diff --git a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java index c2b15bd5f..804539b7c 100644 --- a/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java +++ b/src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderHooksTest.java @@ -77,22 +77,6 @@ void shouldExecuteProviderHooksAndKeepPerProviderContextIsolation() throws Excep assertNull(provider2.lastEvaluationContext.getValue("provider1Marker")); } - @Test - void shouldExposeContextCapturingProviderHook() throws Exception { - HookedStringProvider provider1 = new HookedStringProvider( - "provider1", - List.of(), - ProviderEvaluation.builder().value("ok").build()); - - MultiProvider multiProvider = new MultiProvider(List.of(provider1), new FirstSuccessfulStrategy()); - multiProvider.initialize(null); - - // MultiProvider should expose a provider hook for context capture - var providerHooks = multiProvider.getProviderHooks(); - assertNotNull(providerHooks); - assertEquals(1, providerHooks.size(), "Should have exactly one context-capturing hook"); - } - @Test void shouldPassHookHintsAndClientMetadataAndEnrichThrownProviderErrors() throws Exception { RecordingHook firstHook = new RecordingHook("provider1");