diff --git a/src/main/java/io/getunleash/DefaultUnleash.java b/src/main/java/io/getunleash/DefaultUnleash.java index f9a23364f..0e7771bb2 100644 --- a/src/main/java/io/getunleash/DefaultUnleash.java +++ b/src/main/java/io/getunleash/DefaultUnleash.java @@ -163,56 +163,43 @@ private FeatureEvaluationResult getFeatureEvaluationResult( FeatureToggle featureToggle = featureRepository.getToggle(toggleName); UnleashContext enhancedContext = context.applyStaticFields(config); - Optional 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) { diff --git a/src/main/java/io/getunleash/strategy/Strategy.java b/src/main/java/io/getunleash/strategy/Strategy.java index 694f69977..241ea485f 100644 --- a/src/main/java/io/getunleash/strategy/Strategy.java +++ b/src/main/java/io/getunleash/strategy/Strategy.java @@ -20,9 +20,10 @@ default FeatureEvaluationResult getResult( UnleashContext unleashContext, List constraints, @Nullable List 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 parameters, UnleashContext unleashContext) { diff --git a/src/test/java/io/getunleash/DefaultUnleashTest.java b/src/test/java/io/getunleash/DefaultUnleashTest.java index 4f2205c83..2cb937ccf 100644 --- a/src/test/java/io/getunleash/DefaultUnleashTest.java +++ b/src/test/java/io/getunleash/DefaultUnleashTest.java @@ -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() @@ -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 diff --git a/src/test/java/io/getunleash/UnleashTest.java b/src/test/java/io/getunleash/UnleashTest.java index 1e39d79e8..a47501798 100644 --- a/src/test/java/io/getunleash/UnleashTest.java +++ b/src/test/java/io/getunleash/UnleashTest.java @@ -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.*; @@ -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 = @@ -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 @@ -160,6 +162,45 @@ public void should_support_multiple_strategies() { assertThat(unleash.isEnabled("test")).isTrue(); } + @Test + public void shouldSupportMultipleRolloutStrategies() { + Map 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();