diff --git a/.sampo/changesets/present-properties-match.md b/.sampo/changesets/present-properties-match.md new file mode 100644 index 000000000..a96c55f5a --- /dev/null +++ b/.sampo/changesets/present-properties-match.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +Align local `is_set` and `is_not_set` evaluation with partial property context. diff --git a/posthog/feature_flags.py b/posthog/feature_flags.py index 26612f8db..e6c880d48 100644 --- a/posthog/feature_flags.py +++ b/posthog/feature_flags.py @@ -426,8 +426,10 @@ def match_feature_flag_properties( and match_result == ConditionMatch.OUT_OF_ROLLOUT_BOUND ): # The condition's property filters (if any) matched and only the rollout check - # failed, so re-evaluating later groups can't change the outcome. Return a - # deterministic False, mirroring the server-side engine. + # failed, so re-evaluating later groups can't change the outcome. If an earlier + # condition was inconclusive, stop here but preserve that result for fallback. + if is_inconclusive: + break return False except RequiresServerEvaluation: # Static cohort or other missing server-side data - must fallback to API @@ -509,7 +511,6 @@ def is_condition_match( def match_property(property, property_values) -> bool: # only looks for matches where key exists in override_property_values - # doesn't support operator is_not_set key = property.get("key") operator = property.get("operator") or "exact" value = property.get("value") @@ -522,8 +523,8 @@ def match_property(property, property_values) -> bool: "can't match properties without a given property value" ) - if operator == "is_not_set": - raise InconclusiveMatchError("can't match properties with operator is_not_set") + if operator in ("is_set", "is_not_set"): + return operator == "is_set" override_value = property_values[key] @@ -544,9 +545,6 @@ def compute_exact_match(value, override_value): else: return not compute_exact_match(value, override_value) - if operator == "is_set": - return key in property_values - if operator == "icontains": return utils.str_icontains(override_value, value) diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 5b54ba769..68873f46c 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -362,6 +362,51 @@ def test_early_exit_on_rollout_only_group_with_no_property_filters(self): self.client.get_feature_flag("early-exit-flag", "some-distinct-id") ) + @mock.patch("posthog.client.flags") + def test_early_exit_preserves_fallback_after_inconclusive_presence_condition( + self, patch_flags + ): + patch_flags.return_value = { + "featureFlags": {"early-exit-presence-flag": "server-fallback"} + } + + for operator in ("is_set", "is_not_set"): + with self.subTest(operator=operator): + self.client.feature_flags = [ + { + "id": 1, + "name": "Early Exit Presence Feature", + "key": "early-exit-presence-flag", + "active": True, + "filters": { + "early_exit": True, + "groups": [ + { + "properties": [ + { + "key": "plan", + "operator": operator, + "value": "", + "type": "person", + } + ], + "rollout_percentage": 100, + }, + {"properties": [], "rollout_percentage": 0}, + ], + }, + } + ] + + self.assertEqual( + "server-fallback", + self.client.get_feature_flag( + "early-exit-presence-flag", "some-distinct-id" + ), + ) + + self.assertEqual(patch_flags.call_count, 2) + def test_early_exit_does_not_trigger_on_property_mismatch(self): # First group fails on its property (region mismatch), not rollout — so even with # early_exit enabled, evaluation must continue to the second group, which matches. @@ -5072,16 +5117,33 @@ def test_match_properties_not_in(self): match_property(property_a, {"key2": "value"}) match_property(property_c, {"key2": "value1"}) # overrides don't have 'key' - def test_match_properties_is_set(self): - property_a = self.property(key="key", value="is_set", operator="is_set") - self.assertTrue(match_property(property_a, {"key": "value"})) - self.assertTrue(match_property(property_a, {"key": "value2"})) - self.assertTrue(match_property(property_a, {"key": ""})) - self.assertFalse(match_property(property_a, {"key": None})) + @parameterized.expand( + [ + ("is_set_none", "is_set", None, True), + ("is_set_false", "is_set", False, True), + ("is_set_zero", "is_set", 0, True), + ("is_set_empty_string", "is_set", "", True), + ("is_set_empty_list", "is_set", [], True), + ("is_set_empty_dict", "is_set", {}, True), + ("is_not_set_none", "is_not_set", None, False), + ("is_not_set_false", "is_not_set", False, False), + ("is_not_set_zero", "is_not_set", 0, False), + ("is_not_set_empty_string", "is_not_set", "", False), + ("is_not_set_empty_list", "is_not_set", [], False), + ("is_not_set_empty_dict", "is_not_set", {}, False), + ] + ) + def test_match_properties_presence_operator_with_present_key( + self, _name, operator, value, expected + ): + prop = self.property(key="key", value=operator, operator=operator) + self.assertEqual(expected, match_property(prop, {"key": value})) + @parameterized.expand([("is_set", "is_set"), ("is_not_set", "is_not_set")]) + def test_match_properties_presence_operator_with_omitted_key(self, _name, operator): + prop = self.property(key="key", value=operator, operator=operator) with self.assertRaises(InconclusiveMatchError): - match_property(property_a, {"key2": "value"}) - match_property(property_a, {}) + match_property(prop, {}) def test_match_properties_icontains(self): property_a = self.property(key="key", value="valUe", operator="icontains") @@ -5455,9 +5517,6 @@ def test_none_property_value_with_all_operators(self): self.assertFalse(match_property(property_a, {"key": None})) self.assertTrue(match_property(property_a, {"key": "non"})) - property_b = self.property(key="key", value=None, operator="is_set") - self.assertFalse(match_property(property_b, {"key": None})) - property_c = self.property(key="key", value="no", operator="icontains") self.assertFalse(match_property(property_c, {"key": None})) self.assertFalse(match_property(property_c, {"key": "smh"}))