fix: change seed for variantutils to ensure fair distribution (#220)

Background
After a customer reported that variant distribution seemed skewed we performed some testing and found that since we use the same hash string for both gradual rollout and variant allocation we'd reduced the set of groups we could get to whatever percentage our gradual rollout was set.

Example
Take a gradualRollout of 10%, this will select normalized hashes between 1 and 10, when we then again hash the same string that gave us between 1 and 10, but with modulo 1000 for variants, this will only give us 100 possible groups, instead of the expected 1000.

Fix
Force the normalization to accept a seed, and make sure to use a new seed when normalizing the variant distribution hash.

Worth noting
This will require release 9.0.0, since we're changing the signature of public methods.

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>

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