fix: when using multiple strategies some of them might not be evaluat…

…ed (#211)

* fix: resolve all activation strategies with strategy variants

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<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) {

src/main/java/io/getunleash/strategy/Strategy.java

@@ -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) {

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

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<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();