diff --git a/.sampo/changesets/malformed-flag-dependency-no-match.md b/.sampo/changesets/malformed-flag-dependency-no-match.md new file mode 100644 index 00000000..29a82843 --- /dev/null +++ b/.sampo/changesets/malformed-flag-dependency-no-match.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +Malformed flag-dependency conditions (missing key, null value, or wrong operator) now evaluate locally as no-match (false), matching the server, instead of falling back to the `/flags` endpoint on every evaluation. 7.22.1 made these conditions fall back to the server, which could massively increase billable `/flags` request volume for flag definitions containing legacy/malformed dependency conditions. diff --git a/posthog/feature_flags.py b/posthog/feature_flags.py index a94b6ead..8d95ced3 100644 --- a/posthog/feature_flags.py +++ b/posthog/feature_flags.py @@ -15,9 +15,34 @@ log = logging.getLogger("posthog") +# Tracks (flag_key, reason) pairs already warned about for malformed flag +# dependency conditions, so the warning fires at most once per process instead +# of on every get_feature_flag call. A plain set is safe under the GIL; a rare +# duplicate warning from a race is acceptable. +_warned_malformed_flag_dependencies: set = set() + NONE_VALUES_ALLOWED_OPERATORS = ["is_not"] +def _warn_malformed_flag_dependency_once(flag_key, reason): + """Emit a throttled warning for a malformed flag dependency condition. + + Mirrors the server-side Rust evaluator, which logs before treating the + condition as not matching, so customers can discover broken flag + definitions locally. Deduplicated on (flag_key, reason) to avoid spamming + hot evaluation paths. + """ + key = (flag_key, reason) + if key in _warned_malformed_flag_dependencies: + return + _warned_malformed_flag_dependencies.add(key) + log.warning( + f"Flag dependency condition on '{flag_key or 'unknown'}' is malformed " + f"({reason}); treating as not matching during local evaluation. " + f"Fix this condition in the PostHog UI." + ) + + class ConditionMatch(Enum): """Outcome of evaluating a single condition group. @@ -134,11 +159,14 @@ def evaluate_flag_dependency( Returns: bool: Whether the referenced flag's evaluated value matches the - condition's expected value. + condition's expected value. A malformed condition shape (wrong + operator, missing key, or missing value) is a definitive local + no-match and returns False, mirroring the server-side evaluator. Raises: - InconclusiveMatchError: If the condition is malformed or the chain - cannot be conclusively evaluated locally. + InconclusiveMatchError: If the chain cannot be conclusively evaluated + locally (referenced flag missing, an inconclusive dependency, + circular dependency, or missing evaluation context). """ if flags_by_key is None or evaluation_cache is None: # Cannot evaluate flag dependencies without required context @@ -146,6 +174,23 @@ def evaluate_flag_dependency( f"Cannot evaluate flag dependency on '{property.get('key', 'unknown')}' without flags_by_key and evaluation_cache" ) + # Validate the condition shape before walking the chain. A malformed shape + # is a definitive no-match (return False), mirroring the server-side Rust + # evaluator (match_flag_value_to_flag_filter). Raising InconclusiveMatchError + # here would force a billable /flags network fallback on every evaluation of + # the affected flag. Test `is None`, not truthiness: `False` is a valid + # expected value (the case flag dependencies exist for). + flag_key = property.get("key") + expected_value = property.get("value") + operator = property.get("operator", "exact") + + if operator != "flag_evaluates_to": + _warn_malformed_flag_dependency_once(flag_key, f"invalid operator '{operator}'") + return False + if not flag_key or expected_value is None: + _warn_malformed_flag_dependency_once(flag_key, "missing key or value") + return False + # Check if dependency_chain is present - it should always be provided for flag dependencies if "dependency_chain" not in property: # Missing dependency_chain indicates malformed server data @@ -162,21 +207,6 @@ def evaluate_flag_dependency( f"Circular dependency detected for flag '{property.get('key', 'unknown')}'" ) - # Validate the condition shape before walking the chain. Test `is None`, not - # truthiness: `False` is a valid expected value (the case this exists for). - flag_key = property.get("key") - expected_value = property.get("value") - operator = property.get("operator", "exact") - - if operator != "flag_evaluates_to": - raise InconclusiveMatchError( - f"Flag dependency property for '{flag_key or 'unknown'}' has invalid operator '{operator}'" - ) - if not flag_key or expected_value is None: - raise InconclusiveMatchError( - f"Flag dependency property for '{flag_key or 'unknown'}' is missing a key or value" - ) - # Evaluate and cache each flag in the chain; members already cached are # skipped. This does not decide the outcome — it only populates the cache. for dep_flag_key in dependency_chain: diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index f6c26138..47eba37b 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -9,6 +9,7 @@ from parameterized import parameterized from posthog.client import Client +import posthog.feature_flags from posthog.feature_flags import ( InconclusiveMatchError, match_property, @@ -38,6 +39,9 @@ def set_fail(self, e, batch): def setUp(self): self.failed = False self.client = Client(FAKE_TEST_API_KEY, on_error=self.set_fail) + # Reset the process-wide malformed-dependency warning dedup set so tests + # asserting on the warning stay order-independent. + posthog.feature_flags._warned_malformed_flag_dependencies.clear() @mock.patch("posthog.client.get") def test_flag_person_properties(self, patch_get): @@ -2310,8 +2314,10 @@ def test_flag_dependencies_inconclusive_base_falls_back( @mock.patch("posthog.client.flags") @mock.patch("posthog.client.get") - def test_flag_dependencies_null_value_falls_back(self, patch_get, patch_flags): - """A malformed `value: null` condition is inconclusive and falls back to /flags rather than silently matching.""" + def test_flag_dependencies_null_value_no_match_locally( + self, patch_get, patch_flags + ): + """A malformed `value: null` condition is a definitive local no-match (False), never falling back to /flags.""" patch_flags.return_value = {"featureFlags": {}} client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) client.feature_flags = [ @@ -2346,9 +2352,228 @@ def test_flag_dependencies_null_value_falls_back(self, patch_get, patch_flags): }, ] + # The malformed condition does not match, so the only group fails and the + # flag resolves to False locally - no billable /flags fallback. result = client.get_feature_flag("dependent-flag", "user-1") - self.assertIsNone(result) - self.assertEqual(patch_flags.call_count, 1) + self.assertFalse(result) + self.assertEqual(patch_flags.call_count, 0) + + @mock.patch("posthog.client.flags") + @mock.patch("posthog.client.get") + def test_flag_dependencies_missing_key_no_match_locally( + self, patch_get, patch_flags + ): + """A dependency condition with an empty/missing `key` is a definitive local no-match (False).""" + patch_flags.return_value = {"featureFlags": {}} + client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + client.feature_flags = [ + { + "id": 1, + "name": "Base Flag", + "key": "base-flag", + "active": True, + "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}, + }, + { + "id": 2, + "name": "Dependent Flag", + "key": "dependent-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "", + "operator": "flag_evaluates_to", + "value": True, + "type": "flag", + "dependency_chain": ["base-flag"], + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + ] + + result = client.get_feature_flag("dependent-flag", "user-1") + self.assertFalse(result) + self.assertEqual(patch_flags.call_count, 0) + + @mock.patch("posthog.client.flags") + @mock.patch("posthog.client.get") + def test_flag_dependencies_wrong_operator_no_match_locally( + self, patch_get, patch_flags + ): + """A dependency condition with the wrong operator is a definitive local no-match (False).""" + patch_flags.return_value = {"featureFlags": {}} + client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + client.feature_flags = [ + { + "id": 1, + "name": "Base Flag", + "key": "base-flag", + "active": True, + "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}, + }, + { + "id": 2, + "name": "Dependent Flag", + "key": "dependent-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "base-flag", + "operator": "exact", + "value": True, + "type": "flag", + "dependency_chain": ["base-flag"], + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + ] + + result = client.get_feature_flag("dependent-flag", "user-1") + self.assertFalse(result) + self.assertEqual(patch_flags.call_count, 0) + + @mock.patch("posthog.client.flags") + @mock.patch("posthog.client.get") + def test_flag_dependencies_malformed_condition_warns_once( + self, patch_get, patch_flags + ): + """A malformed dependency condition warns on first eval, then stays quiet (dedup).""" + patch_flags.return_value = {"featureFlags": {}} + client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + client.feature_flags = [ + { + "id": 1, + "name": "Base Flag", + "key": "base-flag", + "active": True, + "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}, + }, + { + "id": 2, + "name": "Dependent Flag", + "key": "dependent-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "base-flag", + "operator": "flag_evaluates_to", + "value": None, + "type": "flag", + "dependency_chain": ["base-flag"], + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + ] + + # First evaluation emits exactly one warning naming the malformed flag. + with self.assertLogs("posthog", level="WARNING") as logs: + self.assertFalse(client.get_feature_flag("dependent-flag", "user-1")) + self.assertEqual(len(logs.records), 1) + self.assertIn("base-flag", logs.output[0]) + self.assertIn("malformed", logs.output[0]) + + # Second evaluation of the same malformed condition stays silent (dedup). + # assertLogs fails if nothing is logged, so log a sentinel to prove the + # malformed-dependency warning itself did not fire again. + with self.assertLogs("posthog", level="WARNING") as logs: + self.assertFalse(client.get_feature_flag("dependent-flag", "user-2")) + posthog.feature_flags.log.warning("sentinel") + self.assertEqual(len(logs.records), 1) + self.assertIn("sentinel", logs.output[0]) + + self.assertEqual(patch_flags.call_count, 0) + + @mock.patch("posthog.client.flags") + @mock.patch("posthog.client.get") + def test_flag_dependencies_malformed_condition_falls_through_to_other_group( + self, patch_get, patch_flags + ): + """A malformed dependency condition no-matches its group, but a second matching group still resolves the flag locally.""" + patch_flags.return_value = {"featureFlags": {}} + client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + client.feature_flags = [ + { + "id": 1, + "name": "Base Flag", + "key": "base-flag", + "active": True, + "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}, + }, + # Only condition is the malformed dependency -> resolves to False locally. + { + "id": 2, + "name": "Malformed Only Flag", + "key": "malformed-only-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "base-flag", + "operator": "flag_evaluates_to", + "value": None, + "type": "flag", + "dependency_chain": ["base-flag"], + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + # Malformed dependency group no-matches, but the second group matches. + { + "id": 3, + "name": "Malformed Plus Matching Flag", + "key": "malformed-plus-matching-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "base-flag", + "operator": "flag_evaluates_to", + "value": None, + "type": "flag", + "dependency_chain": ["base-flag"], + } + ], + "rollout_percentage": 100, + }, + {"properties": [], "rollout_percentage": 100}, + ], + }, + }, + ] + + self.assertFalse(client.get_feature_flag("malformed-only-flag", "user-1")) + self.assertTrue( + client.get_feature_flag("malformed-plus-matching-flag", "user-1") + ) + self.assertEqual(patch_flags.call_count, 0) @mock.patch("posthog.client.flags") @mock.patch("posthog.client.get") @@ -5906,6 +6131,9 @@ def set_fail(self, e, batch): def setUp(self): self.failed = False self.client = Client(FAKE_TEST_API_KEY, on_error=self.set_fail) + # Reset the process-wide malformed-dependency warning dedup set so tests + # asserting on the warning stay order-independent. + posthog.feature_flags._warned_malformed_flag_dependencies.clear() @mock.patch("posthog.client.get") def test_simple_flag_consistency(self, patch_get):