diff --git a/ldclient/flag.py b/ldclient/flag.py index 11a5be41..422a56f0 100644 --- a/ldclient/flag.py +++ b/ldclient/flag.py @@ -198,17 +198,28 @@ def _variation_index_for_user(feature, rule, user): if rule.get('variation') is not None: return rule['variation'] - if rule.get('rollout') is not None: + rollout = rule.get('rollout') + if rollout is None: + return None + variations = rollout.get('variations') + if variations is not None and len(variations) > 0: bucket_by = 'key' - if rule['rollout'].get('bucketBy') is not None: - bucket_by = rule['rollout']['bucketBy'] + if rollout.get('bucketBy') is not None: + bucket_by = rollout['bucketBy'] bucket = _bucket_user(user, feature['key'], feature['salt'], bucket_by) sum = 0.0 - for wv in rule['rollout'].get('variations') or []: + for wv in variations: sum += wv.get('weight', 0.0) / 100000.0 if bucket < sum: return wv.get('variation') + # 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. + return variations[-1].get('variation') + return None diff --git a/testing/test_flag.py b/testing/test_flag.py index ced400e5..6b50b55a 100644 --- a/testing/test_flag.py +++ b/testing/test_flag.py @@ -1,6 +1,7 @@ +import math import pytest from ldclient.feature_store import InMemoryFeatureStore -from ldclient.flag import EvaluationDetail, EvalResult, _bucket_user, evaluate +from ldclient.flag import EvaluationDetail, EvalResult, _bucket_user, _variation_index_for_user, evaluate from ldclient.impl.event_factory import _EventFactory from ldclient.versioned_data_kind import FEATURES, SEGMENTS @@ -384,7 +385,47 @@ def _make_bool_flag_from_clause(clause): 'variations': [ False, True ] } +def test_variation_index_is_returned_for_bucket(): + 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 = math.trunc(_bucket_user(user, flag['key'], flag['salt'], 'key') * 100000) + assert bucket_value > 0 and bucket_value < 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) } + ] + } + } + result_variation = _variation_index_for_user(flag, rule, user) + assert result_variation == matched_variation +def test_last_bucket_is_used_if_bucket_value_equals_total_weight(): + user = { 'key': 'userkey' } + flag = { 'key': 'flagkey', 'salt': 'salt' } + + # We'll construct a list of variations that stops right at the target bucket value + bucket_value = math.trunc(_bucket_user(user, flag['key'], flag['salt'], 'key') * 100000) + + rule = { + 'rollout': { + 'variations': [ + { 'variation': 0, 'weight': bucket_value } + ] + } + } + result_variation = _variation_index_for_user(flag, rule, user) + assert result_variation == 0 + def test_bucket_by_user_key(): user = { u'key': u'userKeyA' } bucket = _bucket_user(user, 'hashKey', 'saltyA', 'key')