fix: change seed for variantutils to ensure fair distribution

pom.xml

@@ -6,14 +6,14 @@
     <version>8.4.1-SNAPSHOT</version>
 
     <properties>
-        <version.slf4j>2.0.4</version.slf4j>
+        <version.slf4j>2.0.9</version.slf4j>
         <version.log4j2>2.19.0</version.log4j2>
-        <version.junit5>5.9.0</version.junit5>
+        <version.junit5>5.10.0</version.junit5>
         <version.okhttp>4.10.0</version.okhttp>
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
-        <version.unleash.specification>4.5.1</version.unleash.specification>
+        <version.unleash.specification>5.0.2</version.unleash.specification>
         <arguments />
-        <version.jackson>2.14.0</version.jackson>
+        <version.jackson>2.14.3</version.jackson>
     </properties>
 
     <name>io.getunleash:unleash-client-java</name>
@@ -31,18 +31,18 @@
     <developers>
         <developer>
             <id>chriswk</id>
-            <email>chriswk@getunleash.ai</email>
+            <email>chriswk@getunleash.io</email>
             <name>Christopher Kolstad</name>
             <url>https://github.com/chriswk</url>
             <organization>Unleash</organization>
-            <organizationUrl>https://getunleash.ai</organizationUrl>
+            <organizationUrl>https://getunleash.io</organizationUrl>
         </developer>
         <developer>
             <id>ivarconr</id>
-            <email>ivar@getunleash.ai</email>
+            <email>ivar@getunleash.io</email>
             <name>Ivar Conradi Østhus</name>
             <url>https://github.com/ivarconr</url>
-            <organizationUrl>https://getunleash.ai</organizationUrl>
+            <organizationUrl>https://getunleash.io</organizationUrl>
             <organization>Unleash</organization>
         </developer>
     </developers>
@@ -58,7 +58,7 @@
         <dependency>
             <groupId>com.google.code.gson</groupId>
             <artifactId>gson</artifactId>
-            <version>2.10</version>
+            <version>2.10.1</version>
         </dependency>
         <dependency>
             <groupId>com.squareup.okhttp3</groupId>
@@ -104,7 +104,7 @@
         <dependency>
             <groupId>org.assertj</groupId>
             <artifactId>assertj-core</artifactId>
-            <version>3.23.1</version>
+            <version>3.24.2</version>
             <scope>test</scope>
         </dependency>
 
@@ -117,7 +117,7 @@
         <dependency>
             <groupId>com.github.tomakehurst</groupId>
             <artifactId>wiremock-jre8</artifactId>
-            <version>2.35.0</version>
+            <version>2.35.1</version>
             <scope>test</scope>
         </dependency>
         <dependency>
@@ -142,13 +142,13 @@
         <dependency>
             <groupId>ch.qos.logback</groupId>
             <artifactId>logback-core</artifactId>
-            <version>1.3.5</version>
+            <version>1.4.8</version>
             <scope>test</scope>
         </dependency>
         <dependency>
             <groupId>ch.qos.logback</groupId>
             <artifactId>logback-classic</artifactId>
-            <version>1.3.5</version>
+            <version>1.4.8</version>
             <scope>test</scope>
         </dependency>
 

src/main/java/io/getunleash/DefaultUnleash.java

@@ -245,7 +245,10 @@ private boolean isParentDependencySatisfied(
                                             parent.getFeature());
                                     return false;
                                 }
-                                if (parent.isEnabled()) {
+                                boolean parentSatisfied =
+                                        isEnabled(
+                                                parent.getFeature(), context, fallbackAction, true);
+                                if (parentSatisfied) {
                                     if (!parent.getVariants().isEmpty()) {
                                         return parent.getVariants()
                                                 .contains(
@@ -255,12 +258,11 @@ private boolean isParentDependencySatisfied(
                                                                         DISABLED_VARIANT,
                                                                         true)
                                                                 .getName());
+                                    } else {
+                                        return parent.isEnabled();
                                     }
-                                    return isEnabled(
-                                            parent.getFeature(), context, fallbackAction, true);
                                 } else {
-                                    return !isEnabled(
-                                            parent.getFeature(), context, fallbackAction, true);
+                                    return !parent.isEnabled();
                                 }
                             });
         }

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

@@ -55,7 +55,7 @@ public boolean isEnabled(Map<String, String> parameters, UnleashContext unleashC
         final String groupId = parameters.getOrDefault(GROUP_ID, "");
 
         return stickinessId
-                .map(stick -> StrategyUtils.getNormalizedNumber(stick, groupId))
+                .map(stick -> StrategyUtils.getNormalizedNumber(stick, groupId, 0))
                 .map(norm -> percentage > 0 && norm <= percentage)
                 .orElse(false);
     }

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

@@ -42,7 +42,8 @@ public boolean isEnabled(final Map<String, String> parameters, UnleashContext un
         final int percentage = StrategyUtils.getPercentage(parameters.get(PERCENTAGE));
         final String groupId = parameters.getOrDefault(GROUP_ID, "");
 
-        final int normalizedSessionId = StrategyUtils.getNormalizedNumber(sessionId.get(), groupId);
+        final int normalizedSessionId =
+                StrategyUtils.getNormalizedNumber(sessionId.get(), groupId, 0);
 
         return percentage > 0 && normalizedSessionId <= percentage;
     }

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

@@ -42,7 +42,7 @@ public boolean isEnabled(final Map<String, String> parameters, UnleashContext un
         final int percentage = StrategyUtils.getPercentage(parameters.get(PERCENTAGE));
         final String groupId = parameters.getOrDefault(GROUP_ID, "");
 
-        final int normalizedUserId = StrategyUtils.getNormalizedNumber(userId.get(), groupId);
+        final int normalizedUserId = StrategyUtils.getNormalizedNumber(userId.get(), groupId, 0);
 
         return percentage > 0 && normalizedUserId <= percentage;
     }

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

@@ -35,13 +35,14 @@ public static boolean isNumeric(final CharSequence cs) {
      * @param groupId
      * @return
      */
-    public static int getNormalizedNumber(String identifier, String groupId) {
-        return getNormalizedNumber(identifier, groupId, ONE_HUNDRED);
+    public static int getNormalizedNumber(String identifier, String groupId, long seed) {
+        return getNormalizedNumber(identifier, groupId, ONE_HUNDRED, seed);
     }
 
-    public static int getNormalizedNumber(String identifier, String groupId, int normalizer) {
+    public static int getNormalizedNumber(
+            String identifier, String groupId, int normalizer, long seed) {
         byte[] value = (groupId + ':' + identifier).getBytes();
-        long hash = Murmur3.hash_x86_32(value, value.length, 0);
+        long hash = Murmur3.hash_x86_32(value, value.length, seed);
         return (int) (hash % normalizer) + 1;
     }
 

src/main/java/io/getunleash/variant/VariantUtil.java

@@ -5,13 +5,16 @@
 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 {
     static final String GROUP_ID_KEY = "groupId";
+    // To avoid using the same seed for gradual rollout and variant selection.
+    // This caused an unfortunate bias since we'd already excluded x % of the hash results.
+    // This is the 5.000.001st prime.
+    public static final Long VARIANT_NORMALIZATION_SEED = 86028157L;
 
     private VariantUtil() {}
 
@@ -115,7 +118,8 @@ public static Variant selectVariant(
                     StrategyUtils.getNormalizedNumber(
                             getSeed(context, customStickiness),
                             parameters.get(GROUP_ID_KEY),
-                            totalWeight);
+                            totalWeight,
+                            VARIANT_NORMALIZATION_SEED);
 
             int counter = 0;
             for (VariantDefinition variant : variants) {

src/test/java/io/getunleash/DependentFeatureToggleTest.java

@@ -1,5 +1,6 @@
 package io.getunleash;
 
+import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.*;
@@ -146,4 +147,33 @@ public void should_trigger_impression_event_for_parent_variant_when_checking_chi
         assertThat(variant).isNotNull();
         verify(eventDispatcher, times(2)).dispatch(any(VariantImpressionEvent.class));
     }
+
+    @Test
+    public void
+            child_is_disabled_if_the_parent_is_disabled_even_if_the_childs_expected_variant_is_the_disabled_variant() {
+        Map<String, String> parameters = new HashMap<>();
+        parameters.put("rollout", "100");
+        parameters.put("stickiness", "default");
+        parameters.put("groupId", "groupId");
+        String parentName = "parent.disabled";
+        FeatureToggle parent =
+                new FeatureToggle(
+                        parentName,
+                        false,
+                        singletonList(new ActivationStrategy("default", new HashMap<>())));
+        String childName = "parent.disabled.child.expects.disabled.variant";
+        FeatureToggle child =
+                new FeatureToggle(
+                        childName,
+                        true,
+                        singletonList(new ActivationStrategy("flexibleRollout", parameters)),
+                        emptyList(),
+                        false,
+                        singletonList(
+                                new FeatureDependency(
+                                        parentName, null, singletonList("disabled"))));
+        when(featureRepository.getToggle(childName)).thenReturn(child);
+        when(featureRepository.getToggle(parentName)).thenReturn(parent);
+        assertThat(sut.isEnabled(childName, UnleashContext.builder().build())).isFalse();
+    }
 }

src/test/java/io/getunleash/UnleashTest.java

@@ -377,11 +377,11 @@ public void get_first_variant() {
 
     @Test
     public void get_second_variant() {
-        UnleashContext context = UnleashContext.builder().userId("111").build();
+        UnleashContext context = UnleashContext.builder().userId("5").build();
 
         // Set up a toggleName using UserWithIdStrategy
         Map<String, String> params = new HashMap<>();
-        params.put("userIds", "123, 111, 121, 13");
+        params.put("userIds", "123, 5, 121, 13");
         ActivationStrategy strategy = new ActivationStrategy("userWithId", params);
         FeatureToggle featureToggle =
                 new FeatureToggle("test", true, asList(strategy), getTestVariants());
@@ -457,12 +457,12 @@ public void get_first_variant_with_context_provider() {
     @Test
     public void get_second_variant_with_context_provider() {
 
-        UnleashContext context = UnleashContext.builder().userId("111").build();
+        UnleashContext context = UnleashContext.builder().userId("5").build();
         when(contextProvider.getContext()).thenReturn(context);
 
         // Set up a toggleName using UserWithIdStrategy
         Map<String, String> params = new HashMap<>();
-        params.put("userIds", "123, 111, 121");
+        params.put("userIds", "123, 5, 121");
         ActivationStrategy strategy = new ActivationStrategy("userWithId", params);
         FeatureToggle featureToggle =
                 new FeatureToggle("test", true, asList(strategy), getTestVariants());

src/test/java/io/getunleash/strategy/GradualRolloutSessionIdStrategyTest.java

@@ -106,7 +106,7 @@ public void should_not_be_enabled_when_0percent_rollout() {
     public void should_be_enabled_above_minimum_percentage() {
         String sessionId = "1574576830";
         String groupId = "";
-        int minimumPercentage = StrategyUtils.getNormalizedNumber(sessionId, groupId);
+        int minimumPercentage = StrategyUtils.getNormalizedNumber(sessionId, groupId, 0);
 
         UnleashContext context = UnleashContext.builder().sessionId(sessionId).build();
 

src/test/java/io/getunleash/strategy/GradualRolloutUserIdStrategyTest.java

@@ -98,7 +98,7 @@ public void should_not_be_enabled_when_0percent_rollout() {
     public void should_be_enabled_above_minimum_percentage() {
         String userId = "1574576830";
         String groupId = "";
-        int minimumPercentage = StrategyUtils.getNormalizedNumber(userId, groupId);
+        int minimumPercentage = StrategyUtils.getNormalizedNumber(userId, groupId, 0);
 
         UnleashContext context = UnleashContext.builder().userId(userId).build();
 

src/test/java/io/getunleash/strategy/StrategyUtilsTest.java

@@ -1,14 +1,60 @@
 package io.getunleash.strategy;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
+import io.getunleash.variant.VariantUtil;
+import java.util.UUID;
+import org.assertj.core.data.Offset;
 import org.junit.jupiter.api.Test;
 
 public class StrategyUtilsTest {
 
     @Test
     public void normalized_values_are_the_same_across_node_java_and_go_clients() {
-        assertEquals(73, StrategyUtils.getNormalizedNumber("123", "gr1"));
-        assertEquals(25, StrategyUtils.getNormalizedNumber("999", "groupX"));
+        assertEquals(73, StrategyUtils.getNormalizedNumber("123", "gr1", 0));
+        assertEquals(25, StrategyUtils.getNormalizedNumber("999", "groupX", 0));
+    }
+
+    @Test
+    public void normalized_values_with_variant_seed_are_the_same_across_node_java() {
+        assertThat(
+                        StrategyUtils.getNormalizedNumber(
+                                "123", "gr1", VariantUtil.VARIANT_NORMALIZATION_SEED))
+                .isEqualTo(96);
+        assertThat(
+                        StrategyUtils.getNormalizedNumber(
+                                "999", "groupX", VariantUtil.VARIANT_NORMALIZATION_SEED))
+                .isEqualTo(60);
+    }
+
+    @Test
+    public void
+            selecting_ten_percent_of_users_and_then_finding_variants_should_still_have_variants_evenly_distributed() {
+        int ones = 0, twos = 0, threes = 0, loopSize = 500000, selectionSize = 0;
+        for (int i = 0; i < loopSize; i++) {
+            String id = UUID.randomUUID().toString();
+            int featureRollout =
+                    StrategyUtils.getNormalizedNumber(id, "feature.name.that.is.quite.long", 0);
+            if (featureRollout < 11) {
+                int variantGroup =
+                        StrategyUtils.getNormalizedNumber(
+                                id,
+                                "feature.name.that.is.quite.long",
+                                1000,
+                                VariantUtil.VARIANT_NORMALIZATION_SEED);
+                if (variantGroup <= 333) {
+                    ones++;
+                } else if (variantGroup <= 666) {
+                    twos++;
+                } else if (variantGroup <= 1000) {
+                    threes++;
+                }
+                selectionSize++;
+            }
+        }
+        assertThat(ones / (double) (selectionSize)).isCloseTo(0.33, Offset.offset(0.01));
+        assertThat(twos / (double) (selectionSize)).isCloseTo(0.33, Offset.offset(0.01));
+        assertThat(threes / (double) (selectionSize)).isCloseTo(0.33, Offset.offset(0.01));
     }
 }