From 7918d99c6b6cbb58651c58ac319a6048ec778539 Mon Sep 17 00:00:00 2001 From: Dustin Byrne Date: Mon, 19 Oct 2026 19:24:54 -0400 Subject: [PATCH] fix(flags): compare numeric string properties numerically --- .../numeric-string-flag-comparisons.md | 5 + posthog/feature_flags.py | 61 ++--- posthog/test/test_feature_flags.py | 223 +++++++++++++++++- 3 files changed, 252 insertions(+), 37 deletions(-) create mode 100644 .sampo/changesets/numeric-string-flag-comparisons.md diff --git a/.sampo/changesets/numeric-string-flag-comparisons.md b/.sampo/changesets/numeric-string-flag-comparisons.md new file mode 100644 index 000000000..b544a8ee4 --- /dev/null +++ b/.sampo/changesets/numeric-string-flag-comparisons.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +Fix local feature flag `gt`, `gte`, `lt`, and `lte` comparisons to compare numeric strings numerically rather than lexicographically. This can change which users match flags targeting numeric-string properties. Only complete, unpadded decimal strings and finite numbers are accepted: invalid properties no longer compare lexicographically, and invalid filter values allow remote evaluation fallback. diff --git a/posthog/feature_flags.py b/posthog/feature_flags.py index 87da025b5..7545306ea 100644 --- a/posthog/feature_flags.py +++ b/posthog/feature_flags.py @@ -643,6 +643,23 @@ def _is_truthy_property_value(value) -> bool: return False +_NUMERIC_STRING_PATTERN = re.compile( + r"[+-]?(?:[0-9]+(?:\.[0-9]*)?|\.[0-9]+)(?:[eE][+-]?[0-9]+)?" +) + + +def _parse_numeric_value(value) -> float | None: + if isinstance(value, bool) or not isinstance(value, (str, int, float, Decimal)): + return None + if isinstance(value, str) and not _NUMERIC_STRING_PATTERN.fullmatch(value): + return None + try: + parsed = float(value) + except (ValueError, OverflowError): + return None + return parsed if math.isfinite(parsed) else None + + def match_property( property, property_values, property_matching_version: int = 1 ) -> bool: @@ -664,6 +681,21 @@ def match_property( override_value = property_values[key] + if operator in NUMERIC_OPERATORS: + parsed_value = _parse_numeric_value(value) + if parsed_value is None: + raise InconclusiveMatchError("The numeric condition is not a finite number") + parsed_override = _parse_numeric_value(override_value) + if parsed_override is None: + return False + if operator == "gt": + return parsed_override > parsed_value + if operator == "gte": + return parsed_override >= parsed_value + if operator == "lt": + return parsed_override < parsed_value + return parsed_override <= parsed_value + if ( operator not in NONE_VALUES_ALLOWED_OPERATORS and operator != "exact" @@ -730,35 +762,6 @@ def compute_exact_match(value, override_value): and re.compile(str(value)).search(str(override_value)) is None ) - if operator in ("gt", "gte", "lt", "lte"): - # :TRICKY: We adjust comparison based on the override value passed in, - # to make sure we handle both numeric and string comparisons appropriately. - def compare(lhs, rhs, operator): - if operator == "gt": - return lhs > rhs - elif operator == "gte": - return lhs >= rhs - elif operator == "lt": - return lhs < rhs - elif operator == "lte": - return lhs <= rhs - else: - raise ValueError(f"Invalid operator: {operator}") - - parsed_value = None - try: - parsed_value = float(value) - except Exception: - pass - - if parsed_value is not None and override_value is not None: - if isinstance(override_value, str): - return compare(override_value, str(value), operator) - else: - return compare(override_value, parsed_value, operator) - else: - return compare(str(override_value), str(value), operator) - if operator in ["is_date_before", "is_date_after"]: try: parsed_date = relative_date_parse_for_feature_flag_matching(str(value)) diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index a126a0092..374cb3259 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -3,6 +3,7 @@ import threading import unittest +from decimal import Decimal from unittest import mock from zoneinfo import ZoneInfo from dateutil import parser @@ -118,6 +119,105 @@ def test_missing_nested_cohort_always_requires_server_evaluation(self): ) +class TestNumericOrderingLocalEvaluation(unittest.TestCase): + def setUp(self): + self.client = Client(FAKE_TEST_API_KEY) + + def tearDown(self): + self.client.shutdown() + + @parameterized.expand([("get_feature_flag",), ("evaluate_flags",)]) + @mock.patch("posthog.client.flags") + @mock.patch.object(Client, "capture") + def test_numeric_ordering_and_remote_fallback(self, api, _capture, remote): + cases = ( + ("gt", 2, {"amount": "10"}, True, False), + ("lt", 30, {"amount": "100"}, False, False), + ("gte", "5", {"amount": "10"}, True, False), + ("lte", "5", {"amount": "10"}, False, False), + ("gt", "5", {"amount": "10px"}, False, False), + ("gt", "5", {"amount": None}, False, False), + ("gt", "5px", {"amount": "10"}, True, True), + ("gt", "5px", {"amount": None}, True, True), + ("gt", "5", {}, True, True), + ("gt", "2", {"amount": Decimal(10)}, True, False), + ("gte", "5", {"amount": Decimal(5)}, True, False), + ("lt", "5", {"amount": Decimal(2)}, True, False), + ("lte", "5", {"amount": Decimal(5)}, True, False), + ( + "gt", + "9007199254740992", + {"amount": Decimal(9007199254740993)}, + False, + False, + ), + ("gte", "0.1", {"amount": Decimal("0.1")}, True, False), + ("lte", "0", {"amount": Decimal("1e-400")}, True, False), + ("gt", "2", {"amount": Decimal("1e10000")}, False, False), + ("gt", "2", {"amount": Decimal("-1e10000")}, False, False), + ("gt", "2", {"amount": Decimal("NaN")}, False, False), + ("gt", "2", {"amount": Decimal("sNaN")}, False, False), + ("gt", "2", {"amount": Decimal("Infinity")}, False, False), + ("gt", "2", {"amount": Decimal("-Infinity")}, False, False), + ) + for operator, condition, properties, expected, fallback in cases: + for local_only in (False, True): + with self.subTest( + operator=operator, + condition=condition, + properties=properties, + local_only=local_only, + ): + self.client.feature_flags = [ + { + "id": 1, + "key": "numeric-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "amount", + "operator": operator, + "value": condition, + "type": "person", + } + ], + "rollout_percentage": 100, + } + ] + }, + } + ] + remote.reset_mock() + remote.return_value = { + "flags": {"numeric-flag": {"enabled": True, "variant": None}} + } + kwargs = { + "person_properties": properties, + "only_evaluate_locally": local_only, + } + if api == "get_feature_flag": + result = self.client.get_feature_flag( + "numeric-flag", + "user-1", + send_feature_flag_events=False, + **kwargs, + ) + else: + result = self.client.evaluate_flags( + "user-1", **kwargs + ).get_flag("numeric-flag") + if fallback and local_only: + self.assertIsNone(result) + else: + self.assertIs(result, expected) + self.assertEqual( + remote.call_count, int(fallback and not local_only) + ) + + class TestLocalEvaluation(unittest.TestCase): @classmethod def setUpClass(cls): @@ -5625,16 +5725,121 @@ def test_match_properties_math_operators(self): property_e = self.property(key="key", value="30", operator="lt") self.assertTrue(match_property(property_e, {"key": "29"})) - # depending on the type of override, we adjust type comparison - self.assertTrue(match_property(property_e, {"key": "100"})) + self.assertFalse(match_property(property_e, {"key": "100"})) self.assertFalse(match_property(property_e, {"key": 100})) property_f = self.property(key="key", value="123aloha", operator="gt") - self.assertFalse(match_property(property_f, {"key": "123"})) - self.assertFalse(match_property(property_f, {"key": 122})) + for actual in ("123", 122, 129): + with self.assertRaises(InconclusiveMatchError): + match_property(property_f, {"key": actual}) + + @parameterized.expand( + [ + ("gt", "10", 2, True), + ("lt", "100", 30, False), + ("gte", "10", "5", True), + ("lte", "10", "5", False), + ("gt", "+1.e1", "+.5e+1", True), + ("lt", "-1.25E+2", "-12.5", True), + ("gte", "-.5", "-5e-1", True), + ("lte", "1e-400", 0, True), + ("gte", "1.7976931348623157e308", 1e308, True), + ("gt", "9007199254740993", "9007199254740992", False), + ("gt", 9007199254740993, 9007199254740992, False), + ("gt", Decimal(9007199254740993), "9007199254740992", False), + ("gte", Decimal("0.1"), "0.1", True), + ("lte", Decimal("1e-400"), "0", True), + ] + ) + def test_numeric_ordering_regressions(self, operator, actual, condition, expected): + for version in (1, 2): + with self.subTest(version=version): + self.assertEqual( + match_property( + self.property("key", condition, operator), + {"key": actual}, + property_matching_version=version, + ), + expected, + ) - # this turns into a string comparison - self.assertTrue(match_property(property_f, {"key": 129})) + def test_numeric_ordering_operand_types_and_boundaries(self): + for operator, expected_results in ( + ("gt", (False, False, True)), + ("gte", (False, True, True)), + ("lt", (True, False, False)), + ("lte", (True, True, False)), + ): + for condition in (5, 5.0, "5"): + prop = self.property("key", condition, operator) + for actuals, expected in zip( + ( + (2, 2.0, "2", Decimal(2)), + (5, 5.0, "5", Decimal(5)), + (10, 10.0, "10", Decimal(10)), + ), + expected_results, + ): + for actual in actuals: + with self.subTest( + operator=operator, condition=condition, actual=actual + ): + self.assertEqual( + match_property(prop, {"key": actual}), expected + ) + + def test_numeric_ordering_invalid_operands(self): + invalid_values = ( + "", + "abc", + "10px", + " 10", + "10 ", + "10\n", + "1_0", + "1,0", + "1e", + "0x10", + "١٠", + "NaN", + "Infinity", + "-inf", + "1e309", + float("nan"), + float("inf"), + float("-inf"), + Decimal("1e10000"), + Decimal("-1e10000"), + Decimal("NaN"), + Decimal("sNaN"), + Decimal("Infinity"), + Decimal("-Infinity"), + 10**400, + True, + False, + None, + [], + [10], + {}, + {"value": 10}, + b"10", + ) + for operator in ("gt", "gte", "lt", "lte"): + for invalid in invalid_values: + with self.subTest(operator=operator, invalid=invalid): + for condition in (5, "5"): + prop = self.property("key", condition, operator) + self.assertFalse(match_property(prop, {"key": invalid})) + prop = self.property("key", invalid, operator) + for actual in (10, "10", None): + with self.assertRaises(InconclusiveMatchError): + match_property(prop, {"key": actual}) + for condition in (5, "5"): + prop = self.property("key", condition, operator) + with self.assertRaises(InconclusiveMatchError): + match_property(prop, {}) + with self.assertRaises(InconclusiveMatchError): + match_property({"key": "key", "operator": operator}, {"key": 10}) def test_match_property_date_operators(self): property_a = self.property( @@ -5889,10 +6094,12 @@ def test_none_property_value_with_all_operators(self): self.assertFalse(match_property(property_f, {"key": None})) property_g = self.property(key="key", value="xyz", operator="gte") - self.assertFalse(match_property(property_g, {"key": None})) + with self.assertRaises(InconclusiveMatchError): + match_property(property_g, {"key": None}) property_h = self.property(key="key", value="Oo", operator="lte") - self.assertFalse(match_property(property_h, {"key": None})) + with self.assertRaises(InconclusiveMatchError): + match_property(property_h, {"key": None}) property_i = self.property( key="key", value="2022-05-01", operator="is_date_before"