diff --git a/posthog/feature_flags.py b/posthog/feature_flags.py index 0fbb8ed..aa4e477 100644 --- a/posthog/feature_flags.py +++ b/posthog/feature_flags.py @@ -13,6 +13,8 @@ log = logging.getLogger("posthog") +NONE_VALUES_ALLOWED_OPERATORS = ["is_not"] + class InconclusiveMatchError(Exception): pass @@ -119,6 +121,9 @@ def match_property(property, property_values) -> bool: override_value = property_values[key] + if (operator not in NONE_VALUES_ALLOWED_OPERATORS) and override_value is None: + return False + if operator in ("exact", "is_not"): def compute_exact_match(value, override_value): diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 63b77ea..90a857e 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -960,6 +960,60 @@ def test_compute_inactive_flags_locally(self, patch_decide, patch_capture): self.assertEqual(patch_decide.call_count, 0) self.assertEqual(patch_capture.call_count, 0) + @mock.patch("posthog.client.decide") + @mock.patch("posthog.client.get") + def test_feature_flags_local_evaluation_None_values(self, patch_get, patch_decide): + client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + client.feature_flags = [ + { + id: 1, + "name": "Beta Feature", + "key": "beta-feature", + "is_simple_flag": True, + "active": True, + "filters": { + "groups": [ + { + "variant": None, + "properties": [ + {"key": "latestBuildVersion", "type": "person", "value": ".+", "operator": "regex"}, + {"key": "latestBuildVersionMajor", "type": "person", "value": "23", "operator": "gt"}, + {"key": "latestBuildVersionMinor", "type": "person", "value": "31", "operator": "gt"}, + {"key": "latestBuildVersionPatch", "type": "person", "value": "0", "operator": "gt"}, + ], + "rollout_percentage": 100, + } + ], + }, + }, + ] + + feature_flag_match = client.get_feature_flag( + "beta-feature", + "some-distinct-id", + person_properties={ + "latestBuildVersion": None, + "latestBuildVersionMajor": None, + "latestBuildVersionMinor": None, + "latestBuildVersionPatch": None, + }, + ) + + self.assertEqual(feature_flag_match, False) + self.assertEqual(patch_decide.call_count, 0) + self.assertEqual(patch_get.call_count, 0) + + feature_flag_match = client.get_feature_flag( + "beta-feature", + "some-distinct-id", + person_properties={ + "latestBuildVersion": "24.32..1", + "latestBuildVersionMajor": "24", + "latestBuildVersionMinor": "32", + "latestBuildVersionPatch": "1", + }, + ) + @mock.patch("posthog.client.decide") @mock.patch("posthog.client.get") def test_feature_flags_local_evaluation_for_cohorts(self, patch_get, patch_decide): @@ -1714,7 +1768,7 @@ def test_match_properties_is_set(self): self.assertTrue(match_property(property_a, {"key": "value"})) self.assertTrue(match_property(property_a, {"key": "value2"})) self.assertTrue(match_property(property_a, {"key": ""})) - self.assertTrue(match_property(property_a, {"key": None})) + self.assertFalse(match_property(property_a, {"key": None})) with self.assertRaises(InconclusiveMatchError): match_property(property_a, {"key2": "value"}) @@ -1980,20 +2034,20 @@ def test_none_property_value_with_all_operators(self): self.assertTrue(match_property(property_a, {"key": "non"})) property_b = self.property(key="key", value=None, operator="is_set") - self.assertTrue(match_property(property_b, {"key": None})) + self.assertFalse(match_property(property_b, {"key": None})) property_c = self.property(key="key", value="no", operator="icontains") - self.assertTrue(match_property(property_c, {"key": None})) + self.assertFalse(match_property(property_c, {"key": None})) self.assertFalse(match_property(property_c, {"key": "smh"})) property_d = self.property(key="key", value="No", operator="regex") - self.assertTrue(match_property(property_d, {"key": None})) + self.assertFalse(match_property(property_d, {"key": None})) property_d_lower_case = self.property(key="key", value="no", operator="regex") self.assertFalse(match_property(property_d_lower_case, {"key": None})) property_e = self.property(key="key", value=1, operator="gt") - self.assertTrue(match_property(property_e, {"key": None})) + self.assertFalse(match_property(property_e, {"key": None})) property_f = self.property(key="key", value=1, operator="lt") self.assertFalse(match_property(property_f, {"key": None})) @@ -2002,15 +2056,13 @@ def test_none_property_value_with_all_operators(self): self.assertFalse(match_property(property_g, {"key": None})) property_h = self.property(key="key", value="Oo", operator="lte") - self.assertTrue(match_property(property_h, {"key": None})) + self.assertFalse(match_property(property_h, {"key": None})) property_i = self.property(key="key", value="2022-05-01", operator="is_date_before") - with self.assertRaises(InconclusiveMatchError): - self.assertFalse(match_property(property_i, {"key": None})) + self.assertFalse(match_property(property_i, {"key": None})) property_j = self.property(key="key", value="2022-05-01", operator="is_date_after") - with self.assertRaises(InconclusiveMatchError): - self.assertFalse(match_property(property_j, {"key": None})) + self.assertFalse(match_property(property_j, {"key": None})) property_k = self.property(key="key", value="2022-05-01", operator="is_date_before") with self.assertRaises(InconclusiveMatchError):