Repository: Unleash/unleash-client-java (java)

Issue:
Strategy check return only the first enabled
### Describe the bug

In the [commit https://github.com/Unleash/unleash-client-java/commit/78a0d33358bb375d6abe8d08de60f704a12961a6](feat: strategy variants) has been introduced a bug.
When retrieving the toggle enabled strategies it is returned only the first. In case of multiple strategy the the toggle will result disabled even when the context is valid.

See into class `src/main/java/io/getunleash/DefaultUnleash.java`
```
        } else {
            enabledStrategy =
                    featureToggle.getStrategies().stream()
                            .filter(
                                    strategy -> {
                                        Strategy configuredStrategy =
                                                getStrategy(strategy.getName());
	@@ -181,13 +184,33 @@ private boolean checkEnabled(
                                        }

                                        return configuredStrategy.isEnabled(
                                                strategy.getParameters(), context);
                                    })
                            .findFirst();
```

### Steps to reproduce the bug

1. Create a toggle with more then one strategy
2. Check if the toggle is enabled having as context valued that meet second or further strategy in the list.
3. Should expect to return enabled true but is false

### Expected behavior

When calling the method `isEnabled` with context that meet one of the strategy should expect `true` as response

### Logs, error output, etc.

_No response_

### Screenshots

_No response_

### Additional context

_No response_

### Unleash version

8.3.0 ( introduced in this commit-78a0d33358bb375d6abe8d08de60f704a12961a6)

### Subscription type

Enterprise

### Hosting type

Hosted by Unleash

### SDK information (language and version)

java

Pull request:
fix: when using multiple strategies some of them might not be evaluated
## About the changes
The code selecting an activation strategy was exiting on a first enable match but the strategy being enabled does not mean the feature will be enabled. We need to iterate and evaluate each one of the strategies before returning.

Closes #210


Patch:
diff --git a/src/main/java/io/getunleash/DefaultUnleash.java b/src/main/java/io/getunleash/DefaultUnleash.java
index f9a23364..0e7771bb 100644
--- a/src/main/java/io/getunleash/DefaultUnleash.java
+++ b/src/main/java/io/getunleash/DefaultUnleash.java
@@ -163,56 +163,43 @@ public class DefaultUnleash implements Unleash {
         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) {
diff --git a/src/main/java/io/getunleash/strategy/Strategy.java b/src/main/java/io/getunleash/strategy/Strategy.java
index 694f6997..241ea485 100644
--- a/src/main/java/io/getunleash/strategy/Strategy.java
+++ b/src/main/java/io/getunleash/strategy/Strategy.java
@@ -20,9 +20,10 @@ public interface Strategy {
             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) {


Test patch:
diff --git a/src/test/java/io/getunleash/DefaultUnleashTest.java b/src/test/java/io/getunleash/DefaultUnleashTest.java
index 4f2205c8..2cb937cc 100644
--- a/src/test/java/io/getunleash/DefaultUnleashTest.java
+++ b/src/test/java/io/getunleash/DefaultUnleashTest.java
@@ -129,6 +129,7 @@ class DefaultUnleashTest {
     @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 @@ class DefaultUnleashTest {
 
         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 1e39d79e..a4750179 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 class UnleashTest {
         // 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 class UnleashTest {
 
         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 class UnleashTest {
         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();