diff --git a/lib/ldclient-rb/evaluation.rb b/lib/ldclient-rb/evaluation.rb index 43a03c23..d0d2aa38 100644 --- a/lib/ldclient-rb/evaluation.rb +++ b/lib/ldclient-rb/evaluation.rb @@ -323,20 +323,28 @@ def clause_match_user_no_segments(clause, user) end def variation_index_for_user(flag, rule, user) - if !rule[:variation].nil? # fixed variation - return rule[:variation] - elsif !rule[:rollout].nil? # percentage rollout + variation = rule[:variation] + return variation if !variation.nil? # fixed variation + rollout = rule[:rollout] + return nil if rollout.nil? + variations = rollout[:variations] + if !variations.nil? && variations.length > 0 # percentage rollout rollout = rule[:rollout] bucket_by = rollout[:bucketBy].nil? ? "key" : rollout[:bucketBy] bucket = bucket_user(user, flag[:key], bucket_by, flag[:salt]) sum = 0; - rollout[:variations].each do |variate| + variations.each do |variate| sum += variate[:weight].to_f / 100000.0 if bucket < sum return variate[:variation] end end - nil + # The user's bucket value was greater than or equal to the end of the last bucket. This could happen due + # to a rounding error, or due to the fact that we are scaling to 100000 rather than 99999, or the flag + # data could contain buckets that don't actually add up to 100000. Rather than returning an error in + # this case (or changing the scaling, which would potentially change the results for *all* users), we + # will simply put the user in the last bucket. + variations[-1][:variation] else # the rule isn't well-formed nil end diff --git a/spec/evaluation_spec.rb b/spec/evaluation_spec.rb index ff4b63f6..2efbd745 100644 --- a/spec/evaluation_spec.rb +++ b/spec/evaluation_spec.rb @@ -560,6 +560,58 @@ def boolean_flag_with_clauses(clauses) end end + describe "variation_index_for_user" do + it "matches bucket" do + user = { key: "userkey" } + flag_key = "flagkey" + salt = "salt" + + # First verify that with our test inputs, the bucket value will be greater than zero and less than 100000, + # so we can construct a rollout whose second bucket just barely contains that value + bucket_value = (bucket_user(user, flag_key, "key", salt) * 100000).truncate() + expect(bucket_value).to be > 0 + expect(bucket_value).to be < 100000 + + bad_variation_a = 0 + matched_variation = 1 + bad_variation_b = 2 + rule = { + rollout: { + variations: [ + { variation: bad_variation_a, weight: bucket_value }, # end of bucket range is not inclusive, so it will *not* match the target value + { variation: matched_variation, weight: 1 }, # size of this bucket is 1, so it only matches that specific value + { variation: bad_variation_b, weight: 100000 - (bucket_value + 1) } + ] + } + } + flag = { key: flag_key, salt: salt } + + result_variation = variation_index_for_user(flag, rule, user) + expect(result_variation).to be matched_variation + end + + it "uses last bucket if bucket value is equal to total weight" do + user = { key: "userkey" } + flag_key = "flagkey" + salt = "salt" + + bucket_value = (bucket_user(user, flag_key, "key", salt) * 100000).truncate() + + # We'll construct a list of variations that stops right at the target bucket value + rule = { + rollout: { + variations: [ + { variation: 0, weight: bucket_value } + ] + } + } + flag = { key: flag_key, salt: salt } + + result_variation = variation_index_for_user(flag, rule, user) + expect(result_variation).to be 0 + end + end + describe "bucket_user" do it "gets expected bucket values for specific keys" do user = { key: "userKeyA" }