Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: when using multiple strategies some of them might not be evaluated #211

Merged
merged 6 commits into from
Sep 4, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 30 additions & 45 deletions src/main/java/io/getunleash/DefaultUnleash.java
Original file line number Diff line number Diff line change
Expand Up @@ -163,56 +163,41 @@ private FeatureEvaluationResult getFeatureEvaluationResult(
FeatureToggle featureToggle = featureRepository.getToggle(toggleName);

UnleashContext enhancedContext = context.applyStaticFields(config);
Optional<ActivationStrategy> enabledStrategy = Optional.empty();
boolean enabled = false;
if (featureToggle == null) {
enabled = fallbackAction.test(toggleName, enhancedContext);
return new FeatureEvaluationResult(fallbackAction.test(toggleName, enhancedContext), defaultVariant);
} else if (!featureToggle.isEnabled()) {
enabled = false;
return new FeatureEvaluationResult(false, defaultVariant);
} else if (featureToggle.getStrategies().size() == 0) {
enabled = true;
return new FeatureEvaluationResult(true, VariantUtil.selectVariant(featureToggle, context, defaultVariant));
} else {
enabledStrategy =
featureToggle.getStrategies().stream()
.filter(
strategy -> {
Strategy configuredStrategy =
getStrategy(strategy.getName());
if (configuredStrategy == UNKNOWN_STRATEGY) {
LOGGER.warn(
"Unable to find matching strategy for toggle:{} strategy:{}",
toggleName,
strategy.getName());
}

return configuredStrategy.isEnabled(
strategy.getParameters(), context);
})
.findFirst();
}
FeatureEvaluationResult result = new FeatureEvaluationResult(enabled, null);
if (enabledStrategy.isPresent()) {
Strategy configuredStrategy = getStrategy(enabledStrategy.get().getName());
result =
configuredStrategy.getResult(
enabledStrategy.get().getParameters(),
enhancedContext,
ConstraintMerger.mergeConstraints(
featureRepository, enabledStrategy.get()),
enabledStrategy.get().getVariants());
}

Variant variant = result.isEnabled() ? result.getVariant() : null;
// If strategy variant is null, look for a variant in the featureToggle
if (variant == null && defaultVariant != null) {
variant =
result.isEnabled()
? VariantUtil.selectVariant(featureToggle, context, defaultVariant)
: defaultVariant;
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);
}
result.setVariant(variant);
return result;
}
}
return new FeatureEvaluationResult(false, defaultVariant);
}
result.setVariant(variant);

return result;
}

private void checkIfToggleMatchesNamePrefix(String toggleName) {
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/io/getunleash/strategy/Strategy.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ default FeatureEvaluationResult getResult(
UnleashContext unleashContext,
List<Constraint> constraints,
@Nullable List<VariantDefinition> variants) {
boolean enabled = isEnabled(parameters, unleashContext, constraints);
return new FeatureEvaluationResult(
isEnabled(parameters, unleashContext, constraints),
VariantUtil.selectVariant(parameters, variants, unleashContext));
enabled,
enabled? VariantUtil.selectVariant(parameters, variants, unleashContext) : null);
}

default boolean isEnabled(Map<String, String> parameters, UnleashContext unleashContext) {
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/io/getunleash/DefaultUnleashTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ public void should_evaluate_segment_collection_with_one_missing_segment_as_false
@Test
public void should_allow_fallback_strategy() {
Strategy fallback = mock(Strategy.class);
when(fallback.getResult(anyMap(), any(), anyList(), anyList())).thenCallRealMethod();

UnleashConfig unleashConfigWithFallback =
UnleashConfig.builder()
Expand All @@ -152,7 +153,7 @@ public void should_allow_fallback_strategy() {

sut.isEnabled("toggle1");

verify(fallback).isEnabled(any(), any());
verify(fallback).isEnabled(any(), any(), anyList());
}

@Test
Expand Down
43 changes: 42 additions & 1 deletion src/test/java/io/getunleash/UnleashTest.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.getunleash;

import static java.util.Arrays.asList;
import static java.util.Collections.singletonList;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.*;
import static org.mockito.Mockito.*;
Expand Down Expand Up @@ -129,6 +130,7 @@ public void should_register_custom_strategies() {
// custom strategy
Strategy customStrategy = mock(Strategy.class);
when(customStrategy.getName()).thenReturn("custom");
when(customStrategy.getResult(anyMap(), any(), anyList(), anyList())).thenCallRealMethod();

// register custom strategy
UnleashConfig config =
Expand All @@ -144,7 +146,7 @@ public void should_register_custom_strategies() {

unleash.isEnabled("test");

verify(customStrategy, times(1)).isEnabled(any(), any(UnleashContext.class));
verify(customStrategy, times(1)).isEnabled(any(), any(UnleashContext.class), anyList());
}

@Test
Expand All @@ -160,6 +162,45 @@ public void should_support_multiple_strategies() {
assertThat(unleash.isEnabled("test")).isTrue();
}

@Test
public void should_support_multiple_rollout_strategies() {
Map<String, String> rollout100percent = new HashMap<>();
rollout100percent.put("rollout", "100");
rollout100percent.put("stickiness", "default");
rollout100percent.put("groupId", "rollout");

Constraint user6Constraint = new Constraint("userId", Operator.IN, singletonList("6"));
Constraint user9Constraint = new Constraint("userId", Operator.IN, singletonList("9"));

ActivationStrategy strategy1 =
new ActivationStrategy(
"flexibleRollout",
rollout100percent,
singletonList(user6Constraint),
null,
null);
ActivationStrategy strategy2 =
new ActivationStrategy(
"flexibleRollout",
rollout100percent,
singletonList(user9Constraint),
null,
null);

FeatureToggle featureToggle = new FeatureToggle("test", true, asList(strategy1, strategy2));

when(toggleRepository.getToggle("test")).thenReturn(featureToggle);

assertThat(unleash.isEnabled("test", UnleashContext.builder().userId("1").build()))
.isFalse();
assertThat(unleash.isEnabled("test", UnleashContext.builder().userId("6").build()))
.isTrue();
assertThat(unleash.isEnabled("test", UnleashContext.builder().userId("7").build()))
.isFalse();
assertThat(unleash.isEnabled("test", UnleashContext.builder().userId("9").build()))
.isTrue();
}

@Test
public void should_support_context_provider() {
UnleashContext context = UnleashContext.builder().userId("111").build();
Expand Down