From f74e2524cce7aadfda77acc6bee5365624f2e1c4 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Thu, 12 Oct 2023 13:58:15 +0200 Subject: [PATCH] feat: Handle dependent feature toggles (#218) * feat: Handle dependent feature toggles --- pom.xml | 2 +- .../io/getunleash/ActivationStrategy.java | 3 + .../java/io/getunleash/DefaultUnleash.java | 125 +++++++++++---- .../java/io/getunleash/FeatureDependency.java | 51 ++++++ .../java/io/getunleash/FeatureToggle.java | 44 +++++- .../io/getunleash/variant/VariantUtil.java | 2 + .../io/getunleash/DefaultUnleashTest.java | 5 +- .../DependentFeatureToggleTest.java | 149 ++++++++++++++++++ 8 files changed, 343 insertions(+), 38 deletions(-) create mode 100644 src/main/java/io/getunleash/FeatureDependency.java create mode 100644 src/test/java/io/getunleash/DependentFeatureToggleTest.java diff --git a/pom.xml b/pom.xml index 952606972..7d15dcbd6 100644 --- a/pom.xml +++ b/pom.xml @@ -11,7 +11,7 @@ 5.9.0 4.10.0 UTF-8 - 4.3.0 + 4.5.1 2.14.0 diff --git a/src/main/java/io/getunleash/ActivationStrategy.java b/src/main/java/io/getunleash/ActivationStrategy.java index 72b5de983..1e716bbbc 100644 --- a/src/main/java/io/getunleash/ActivationStrategy.java +++ b/src/main/java/io/getunleash/ActivationStrategy.java @@ -3,6 +3,7 @@ import io.getunleash.lang.Nullable; import io.getunleash.variant.VariantDefinition; import java.util.*; +import javax.annotation.Nonnull; public final class ActivationStrategy { private final String name; @@ -49,7 +50,9 @@ public List getConstraints() { return constraints; } + @Nonnull public List getVariants() { + return variants; } } diff --git a/src/main/java/io/getunleash/DefaultUnleash.java b/src/main/java/io/getunleash/DefaultUnleash.java index 0e7771bb2..a6e53ca6c 100644 --- a/src/main/java/io/getunleash/DefaultUnleash.java +++ b/src/main/java/io/getunleash/DefaultUnleash.java @@ -18,6 +18,7 @@ import java.util.concurrent.atomic.LongAdder; import java.util.function.BiPredicate; import java.util.stream.Collectors; +import javax.annotation.Nonnull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -138,9 +139,19 @@ public boolean isEnabled( String toggleName, UnleashContext context, BiPredicate fallbackAction) { + return isEnabled(toggleName, context, fallbackAction, false); + } + + public boolean isEnabled( + String toggleName, + UnleashContext context, + BiPredicate fallbackAction, + boolean isParent) { FeatureEvaluationResult result = getFeatureEvaluationResult(toggleName, context, fallbackAction, null); - count(toggleName, result.isEnabled()); + if (!isParent) { + count(toggleName, result.isEnabled()); + } eventDispatcher.dispatch(new ToggleEvaluated(toggleName, result.isEnabled())); dispatchEnabledImpressionDataIfNeeded("isEnabled", toggleName, result.isEnabled(), context); return result.isEnabled(); @@ -168,40 +179,93 @@ private FeatureEvaluationResult getFeatureEvaluationResult( fallbackAction.test(toggleName, enhancedContext), defaultVariant); } else if (!featureToggle.isEnabled()) { return new FeatureEvaluationResult(false, defaultVariant); - } else if (featureToggle.getStrategies().size() == 0) { + } else if (featureToggle.getStrategies().isEmpty()) { return new FeatureEvaluationResult( true, VariantUtil.selectVariant(featureToggle, context, defaultVariant)); } else { - for (ActivationStrategy strategy : featureToggle.getStrategies()) { - Strategy configuredStrategy = getStrategy(strategy.getName()); - if (configuredStrategy == UNKNOWN_STRATEGY) { - LOGGER.warn( - "Unable to find matching strategy for toggle:{} strategy:{}", - toggleName, - strategy.getName()); - } + // Dependent toggles, no point in evaluating child strategies if our dependencies are + // not satisfied + if (isParentDependencySatisfied(featureToggle, context, fallbackAction)) { + for (ActivationStrategy strategy : featureToggle.getStrategies()) { + Strategy configuredStrategy = getStrategy(strategy.getName()); + if (configuredStrategy == UNKNOWN_STRATEGY) { + LOGGER.warn( + "Unable to find matching strategy for toggle:{} strategy:{}", + toggleName, + strategy.getName()); + } - FeatureEvaluationResult result = - configuredStrategy.getResult( - strategy.getParameters(), - enhancedContext, - ConstraintMerger.mergeConstraints(featureRepository, strategy), - strategy.getVariants()); - - if (result.isEnabled()) { - Variant variant = result.getVariant(); - // If strategy variant is null, look for a variant in the featureToggle - if (variant == null) { - variant = VariantUtil.selectVariant(featureToggle, context, defaultVariant); + FeatureEvaluationResult result = + configuredStrategy.getResult( + strategy.getParameters(), + enhancedContext, + ConstraintMerger.mergeConstraints(featureRepository, strategy), + strategy.getVariants()); + + if (result.isEnabled()) { + Variant variant = result.getVariant(); + // If strategy variant is null, look for a variant in the featureToggle + if (variant == null) { + variant = + VariantUtil.selectVariant( + featureToggle, context, defaultVariant); + } + result.setVariant(variant); + return result; } - result.setVariant(variant); - return result; } } return new FeatureEvaluationResult(false, defaultVariant); } } + private boolean isParentDependencySatisfied( + @Nonnull FeatureToggle featureToggle, + @Nonnull UnleashContext context, + BiPredicate fallbackAction) { + if (!featureToggle.hasDependencies()) { + return true; + } else { + return featureToggle.getDependencies().stream() + .allMatch( + parent -> { + FeatureToggle parentToggle = + featureRepository.getToggle(parent.getFeature()); + if (parentToggle == null) { + LOGGER.warn( + "Missing dependency [{}] for toggle: [{}]", + parent.getFeature(), + featureToggle.getName()); + return false; + } + if (!parentToggle.getDependencies().isEmpty()) { + LOGGER.warn( + "[{}] depends on feature [{}] which also depends on something. We don't currently support more than one level of dependency resolution", + featureToggle.getName(), + parent.getFeature()); + return false; + } + if (parent.isEnabled()) { + if (!parent.getVariants().isEmpty()) { + return parent.getVariants() + .contains( + getVariant( + parent.feature, + context, + DISABLED_VARIANT, + true) + .getName()); + } + return isEnabled( + parent.getFeature(), context, fallbackAction, true); + } else { + return !isEnabled( + parent.getFeature(), context, fallbackAction, true); + } + }); + } + } + private void checkIfToggleMatchesNamePrefix(String toggleName) { if (config.getNamePrefix() != null) { if (!toggleName.startsWith(config.getNamePrefix())) { @@ -220,12 +284,19 @@ public Variant getVariant(String toggleName, UnleashContext context) { @Override public Variant getVariant(String toggleName, UnleashContext context, Variant defaultValue) { + return getVariant(toggleName, context, defaultValue, false); + } + + private Variant getVariant( + String toggleName, UnleashContext context, Variant defaultValue, boolean isParent) { FeatureEvaluationResult result = getFeatureEvaluationResult(toggleName, context, (n, c) -> false, defaultValue); Variant variant = result.getVariant(); - metricService.countVariant(toggleName, variant.getName()); - // Should count yes/no also when getting variant. - metricService.count(toggleName, result.isEnabled()); + if (!isParent) { + metricService.countVariant(toggleName, variant.getName()); + // Should count yes/no also when getting variant. + metricService.count(toggleName, result.isEnabled()); + } dispatchVariantImpressionDataIfNeeded( toggleName, variant.getName(), result.isEnabled(), context); return variant; diff --git a/src/main/java/io/getunleash/FeatureDependency.java b/src/main/java/io/getunleash/FeatureDependency.java new file mode 100644 index 000000000..5df426f0b --- /dev/null +++ b/src/main/java/io/getunleash/FeatureDependency.java @@ -0,0 +1,51 @@ +package io.getunleash; + +import io.getunleash.lang.Nullable; +import java.util.Collections; +import java.util.List; +import javax.annotation.Nonnull; + +public class FeatureDependency { + public String feature; + @Nullable public Boolean enabled; + @Nullable public List variants; + + public FeatureDependency(String feature) { + this.feature = feature; + } + + public FeatureDependency( + String feature, @Nullable Boolean enabled, @Nullable List variants) { + this.feature = feature; + this.enabled = enabled; + this.variants = variants; + } + + public String getFeature() { + return feature; + } + + public void setFeature(String feature) { + this.feature = feature; + } + + public boolean isEnabled() { + return enabled == null || enabled; // Default value here should be true + } + + public void setEnabled(@Nullable Boolean enabled) { + this.enabled = enabled; + } + + @Nonnull + public List getVariants() { + if (variants != null) { + return variants; + } + return Collections.emptyList(); + } + + public void setVariants(@Nullable List variants) { + this.variants = variants; + } +} diff --git a/src/main/java/io/getunleash/FeatureToggle.java b/src/main/java/io/getunleash/FeatureToggle.java index 44c5ced7d..f2df45b5c 100644 --- a/src/main/java/io/getunleash/FeatureToggle.java +++ b/src/main/java/io/getunleash/FeatureToggle.java @@ -6,6 +6,7 @@ import io.getunleash.variant.VariantDefinition; import java.util.Collections; import java.util.List; +import javax.annotation.Nonnull; public final class FeatureToggle { private final String name; @@ -14,8 +15,10 @@ public final class FeatureToggle { @Nullable private final List variants; private final boolean impressionData; + @Nullable private final List dependencies; + public FeatureToggle(String name, boolean enabled, List strategies) { - this(name, enabled, strategies, emptyList(), false); + this(name, enabled, strategies, emptyList(), false, emptyList()); } public FeatureToggle( @@ -23,7 +26,7 @@ public FeatureToggle( boolean enabled, List strategies, List variants) { - this(name, enabled, strategies, variants, false); + this(name, enabled, strategies, variants, false, emptyList()); } public FeatureToggle( @@ -32,11 +35,22 @@ public FeatureToggle( List strategies, @Nullable List variants, @Nullable Boolean impressionData) { + this(name, enabled, strategies, variants, impressionData, emptyList()); + } + + public FeatureToggle( + String name, + boolean enabled, + List strategies, + @Nullable List variants, + @Nullable Boolean impressionData, + @Nullable List dependencies) { this.name = name; this.enabled = enabled; this.strategies = strategies; this.variants = variants; this.impressionData = impressionData != null ? impressionData : false; + this.dependencies = dependencies; } public String getName() { @@ -47,10 +61,15 @@ public boolean isEnabled() { return enabled; } + @Nonnull public List getStrategies() { + if (strategies == null) { + return Collections.emptyList(); + } return this.strategies; } + @Nonnull public List getVariants() { if (variants == null) { return Collections.emptyList(); @@ -59,6 +78,19 @@ public List getVariants() { } } + @Nonnull + public List getDependencies() { + if (dependencies == null) { + return Collections.emptyList(); + } else { + return dependencies; + } + } + + public boolean hasDependencies() { + return dependencies != null && !dependencies.isEmpty(); + } + @Nullable public boolean hasImpressionData() { return impressionData; @@ -72,14 +104,14 @@ public String toString() { + '\'' + ", enabled=" + enabled - + ", strategies='" + + ", strategies=" + strategies - + '\'' - + ", variants='" + + ", variants=" + variants + ", impressionData=" + impressionData - + '\'' + + ", dependencies=" + + dependencies + '}'; } } diff --git a/src/main/java/io/getunleash/variant/VariantUtil.java b/src/main/java/io/getunleash/variant/VariantUtil.java index 969cd3fcd..15dbafe24 100644 --- a/src/main/java/io/getunleash/variant/VariantUtil.java +++ b/src/main/java/io/getunleash/variant/VariantUtil.java @@ -5,7 +5,9 @@ import io.getunleash.Variant; import io.getunleash.lang.Nullable; import io.getunleash.strategy.StrategyUtils; +import java.awt.*; import java.util.*; +import java.util.List; import java.util.function.Predicate; public final class VariantUtil { diff --git a/src/test/java/io/getunleash/DefaultUnleashTest.java b/src/test/java/io/getunleash/DefaultUnleashTest.java index 2cb937ccf..2f620735e 100644 --- a/src/test/java/io/getunleash/DefaultUnleashTest.java +++ b/src/test/java/io/getunleash/DefaultUnleashTest.java @@ -14,10 +14,7 @@ import io.getunleash.strategy.DefaultStrategy; import io.getunleash.strategy.Strategy; import io.getunleash.util.UnleashConfig; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; +import java.util.*; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.slf4j.LoggerFactory; diff --git a/src/test/java/io/getunleash/DependentFeatureToggleTest.java b/src/test/java/io/getunleash/DependentFeatureToggleTest.java new file mode 100644 index 000000000..49d8d6dc8 --- /dev/null +++ b/src/test/java/io/getunleash/DependentFeatureToggleTest.java @@ -0,0 +1,149 @@ +package io.getunleash; + +import static java.util.Collections.singletonList; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.*; + +import io.getunleash.event.EventDispatcher; +import io.getunleash.event.IsEnabledImpressionEvent; +import io.getunleash.event.VariantImpressionEvent; +import io.getunleash.metric.UnleashMetricService; +import io.getunleash.repository.FeatureRepository; +import io.getunleash.strategy.DefaultStrategy; +import io.getunleash.strategy.Strategy; +import io.getunleash.util.UnleashConfig; +import io.getunleash.variant.VariantDefinition; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class DependentFeatureToggleTest { + private DefaultUnleash sut; + private FeatureRepository featureRepository; + private UnleashContextProvider contextProvider; + private EventDispatcher eventDispatcher; + private UnleashMetricService metricService; + + @BeforeEach + public void setup() { + UnleashConfig unleashConfig = + UnleashConfig.builder().unleashAPI("http://fakeAPI").appName("fakeApp").build(); + featureRepository = mock(FeatureRepository.class); + Map strategyMap = new HashMap<>(); + strategyMap.put("default", new DefaultStrategy()); + contextProvider = mock(UnleashContextProvider.class); + eventDispatcher = mock(EventDispatcher.class); + metricService = mock(UnleashMetricService.class); + + sut = + new DefaultUnleash( + unleashConfig, + featureRepository, + strategyMap, + contextProvider, + eventDispatcher, + metricService); + } + + @Test + public void should_not_increment_count_for_parent_toggle_when_checking_child_toggle() { + FeatureToggle child = + new FeatureToggle( + "child", + true, + singletonList(new ActivationStrategy("default", null)), + Collections.emptyList(), + false, + singletonList(new FeatureDependency("parent"))); + FeatureToggle parent = + new FeatureToggle( + "parent", + true, + singletonList(new ActivationStrategy("default", null)), + Collections.emptyList(), + false); + when(featureRepository.getToggle("child")).thenReturn(child); + when(featureRepository.getToggle("parent")).thenReturn(parent); + boolean enabled = sut.isEnabled("child", UnleashContext.builder().userId("7").build()); + assertThat(enabled).isTrue(); + verify(metricService).count("child", true); + verify(metricService, never()).count(eq("parent"), anyBoolean()); + } + + @Test + public void should_not_increment_count_for_parent_toggle_when_checking_parent_variants() { + FeatureToggle child = + new FeatureToggle( + "child", + true, + singletonList(new ActivationStrategy("default", null)), + singletonList(new VariantDefinition("childVariant", 1, null, null)), + false, + singletonList( + new FeatureDependency("parent", true, singletonList("first")))); + FeatureToggle parent = + new FeatureToggle( + "parent", + true, + singletonList(new ActivationStrategy("default", null)), + singletonList(new VariantDefinition("first", 1, null, null, null)), + false); + when(featureRepository.getToggle("child")).thenReturn(child); + when(featureRepository.getToggle("parent")).thenReturn(parent); + Variant variant = sut.getVariant("child", UnleashContext.builder().userId("7").build()); + assertThat(variant).isNotNull(); + verify(metricService).countVariant("child", "childVariant"); + verify(metricService, never()).countVariant("parent", "first"); + } + + @Test + public void should_trigger_impression_event_for_parent_toggle_when_checking_child_toggle() { + FeatureToggle child = + new FeatureToggle( + "child", + true, + singletonList(new ActivationStrategy("default", null)), + Collections.emptyList(), + false, + singletonList(new FeatureDependency("parent"))); + FeatureToggle parent = + new FeatureToggle( + "parent", + true, + singletonList(new ActivationStrategy("default", null)), + Collections.emptyList(), + true); + when(featureRepository.getToggle("child")).thenReturn(child); + when(featureRepository.getToggle("parent")).thenReturn(parent); + boolean enabled = sut.isEnabled("child", UnleashContext.builder().userId("7").build()); + assertThat(enabled).isTrue(); + verify(eventDispatcher).dispatch(any(IsEnabledImpressionEvent.class)); + } + + @Test + public void should_trigger_impression_event_for_parent_variant_when_checking_child_toggle() { + FeatureToggle child = + new FeatureToggle( + "child", + true, + singletonList(new ActivationStrategy("default", null)), + singletonList(new VariantDefinition("childVariant", 1, null, null)), + true, + singletonList( + new FeatureDependency("parent", true, singletonList("first")))); + FeatureToggle parent = + new FeatureToggle( + "parent", + true, + singletonList(new ActivationStrategy("default", null)), + singletonList(new VariantDefinition("first", 1, null, null, null)), + true); + when(featureRepository.getToggle("child")).thenReturn(child); + when(featureRepository.getToggle("parent")).thenReturn(parent); + Variant variant = sut.getVariant("child", UnleashContext.builder().userId("7").build()); + assertThat(variant).isNotNull(); + verify(eventDispatcher, times(2)).dispatch(any(VariantImpressionEvent.class)); + } +}