Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Python UFC SDK updates #29

Merged
merged 2 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 35 additions & 6 deletions eppo_client/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,15 @@ def get_numeric_assignment(
default: float,
subject_attributes: Optional[SubjectAttributes] = None,
) -> float:
return self.get_assignment_variation(
subject_key,
flag_key,
default,
subject_attributes,
VariationType.NUMERIC,
# convert to float in case we get an int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Took me a moment to understand – I first thought you were going to rescue a mismatch (getNumeric for an Integer flag) which I didn't like. But what you're doing is ensuring that 3.0 (a valid variation for a numeric flag) is indeed returned as a float and not as the integer 3.

return float(
self.get_assignment_variation(
subject_key,
flag_key,
default,
subject_attributes,
VariationType.NUMERIC,
)
)

def get_boolean_assignment(
Expand Down Expand Up @@ -182,6 +185,15 @@ def get_assignment_detail(

result = self.__evaluator.evaluate_flag(flag, subject_key, subject_attributes)

if result.variation and not check_value_type_match(
expected_variation_type, result.variation.value
):
logger.error(
"[Eppo SDK] Variation value does not have the correct type for the flag: "
f"{flag_key} and variation key {result.variation.key}"
)
return None

assignment_event = {
**(result.extra_logging if result else {}),
"allocation": result.allocation_key if result else None,
Expand Down Expand Up @@ -227,3 +239,20 @@ def check_type_match(
expected_type: Optional[VariationType], actual_type: VariationType
):
return expected_type is None or actual_type == expected_type


def check_value_type_match(
expected_type: Optional[VariationType], value: ValueType
) -> bool:
if expected_type is None:
return True
if expected_type in [VariationType.JSON, VariationType.STRING]:
return isinstance(value, str)
if expected_type == VariationType.INTEGER:
return isinstance(value, int)
if expected_type == VariationType.NUMERIC:
# we can convert int to float
return isinstance(value, float) or isinstance(value, int)
Comment on lines +254 to +255
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or isinstance(value, int) seems unnecessary? With this PR we always convert to float? Other than in one of the tests where I see that you explicitly rely on this behavior. In any case this is fine to keep - it's more that I want to check my understanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the or this check would fail for an integer value such as 1, which we can convert to a float

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it now. The cast to float is done after validating the value type, so this is indeed necessary.

if expected_type == VariationType.BOOLEAN:
return isinstance(value, bool)
return False
15 changes: 9 additions & 6 deletions eppo_client/eval.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,8 @@ def evaluate_flag(
if allocation.end_at and now > allocation.end_at:
continue

# Skip allocations when none of the rules match
# So we look for (rule 1) OR (rule 2) OR (rule 3) etc.
# If there are no rules, then we always match
if not allocation.rules or any(
matches_rule(rule, {"id": subject_key, **subject_attributes})
for rule in allocation.rules
if matches_rules(
allocation.rules, {"id": subject_key, **subject_attributes}
):
for split in allocation.splits:
# Split needs to match all shards
Expand Down Expand Up @@ -86,6 +82,13 @@ def hash_key(salt: str, subject_key: str) -> str:
return f"{salt}-{subject_key}"


def matches_rules(rules, subject_attributes):
# Skip allocations when none of the rules match
# So we look for (rule 1) OR (rule 2) OR (rule 3) etc.
# If there are no rules, then we always match
return not rules or any(matches_rule(rule, subject_attributes) for rule in rules)


def none_result(
flag_key: str,
variation_type: VariationType,
Expand Down
6 changes: 6 additions & 0 deletions eppo_client/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class OperatorType(Enum):
LT = "LT"
ONE_OF = "ONE_OF"
NOT_ONE_OF = "NOT_ONE_OF"
IS_NULL = "IS_NULL"


class Condition(SdkBaseModel):
Expand All @@ -40,6 +41,11 @@ def evaluate_condition(
condition: Condition, subject_attributes: SubjectAttributes
) -> bool:
subject_value = subject_attributes.get(condition.attribute, None)
if condition.operator == OperatorType.IS_NULL:
if condition.value:
return subject_value is None
return subject_value is not None
Comment on lines +45 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supernit, optional: could write as return (subject_value is None) if condition.value else (subject_value is not None)


if subject_value is not None:
if condition.operator == OperatorType.MATCHES:
return isinstance(condition.value, str) and bool(
Expand Down
15 changes: 14 additions & 1 deletion test/client_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import httpretty # type: ignore
import pytest
from eppo_client.assignment_logger import AssignmentLogger
from eppo_client.client import EppoClient, check_type_match
from eppo_client.client import EppoClient, check_type_match, check_value_type_match
from eppo_client.config import Config
from eppo_client.models import (
Allocation,
Expand Down Expand Up @@ -270,3 +270,16 @@ def test_get_numeric_assignment_on_bool_feature_flag_should_return_none(test_cas
def test_check_type_match():
assert check_type_match(VariationType.STRING, VariationType.STRING)
assert check_type_match(None, VariationType.STRING)


def test_check_value_type_match():
assert check_value_type_match(VariationType.STRING, "hello")
assert check_value_type_match(VariationType.INTEGER, 1)
assert check_value_type_match(VariationType.NUMERIC, 1.0)
assert check_value_type_match(VariationType.NUMERIC, 1)
assert check_value_type_match(VariationType.BOOLEAN, True)
assert check_value_type_match(VariationType.JSON, '{"hello": "world"}')

assert not check_type_match(VariationType.STRING, 1)
assert not check_type_match(VariationType.INTEGER, 1.0)
assert not check_type_match(VariationType.BOOLEAN, "true")
48 changes: 47 additions & 1 deletion test/eval_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,13 @@
Split,
Shard,
)
from eppo_client.eval import Evaluator, FlagEvaluation, is_in_shard_range, hash_key
from eppo_client.eval import (
Evaluator,
FlagEvaluation,
is_in_shard_range,
hash_key,
matches_rules,
)
from eppo_client.rules import Condition, OperatorType, Rule
from eppo_client.sharders import DeterministicSharder, MD5Sharder

Expand Down Expand Up @@ -424,6 +430,46 @@ def test_eval_after_alloc(mocker):
assert result.variation is None


def test_matches_rules_empty():
rules = []
subject_attributes = {"size": 10}
assert matches_rules(rules, subject_attributes)


def test_matches_rules_with_conditions():
rules = [
Rule(
conditions=[
Condition(attribute="size", operator=OperatorType.IS_NULL, value=True)
]
),
Rule(
conditions=[
Condition(
attribute="country", operator=OperatorType.ONE_OF, value=["UK"]
)
]
),
]
subject_attributes_1 = {"size": None, "country": "US"}
subject_attributes_2 = {"size": 10, "country": "UK"}
subject_attributes_3 = {"size": 5, "country": "US"}
subject_attributes_4 = {"country": "US"}

assert (
matches_rules(rules, subject_attributes_1) is True
), f"{subject_attributes_1} should match first rule"
assert (
matches_rules(rules, subject_attributes_2) is True
), f"{subject_attributes_2} should match second rule"
assert (
matches_rules(rules, subject_attributes_3) is False
), f"{subject_attributes_3} should not match any rule"
assert (
matches_rules(rules, subject_attributes_4) is True
), f"{subject_attributes_4} should match first rule"


def test_seed():
assert hash_key("salt", "subject") == "salt-subject"

Expand Down
18 changes: 18 additions & 0 deletions test/rules_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,3 +255,21 @@ def test_one_of_operator_with_number():
assert not evaluate_condition(not_one_of_condition, {"number": 10})
assert evaluate_condition(not_one_of_condition, {"number": "11"})
assert evaluate_condition(not_one_of_condition, {"number": 11})


def test_is_null_operator():
is_null_condition = Condition(
operator=OperatorType.IS_NULL, value=True, attribute="size"
)
assert evaluate_condition(is_null_condition, {"size": None})
assert not evaluate_condition(is_null_condition, {"size": 10})
assert evaluate_condition(is_null_condition, {})


def test_is_not_null_operator():
is_not_null_condition = Condition(
operator=OperatorType.IS_NULL, value=False, attribute="size"
)
assert not evaluate_condition(is_not_null_condition, {"size": None})
assert evaluate_condition(is_not_null_condition, {"size": 10})
assert not evaluate_condition(is_not_null_condition, {})
Loading