diff --git a/.sampo/changesets/parse-locally-evaluated-flag-payloads.md b/.sampo/changesets/parse-locally-evaluated-flag-payloads.md new file mode 100644 index 00000000..7ed2b1d7 --- /dev/null +++ b/.sampo/changesets/parse-locally-evaluated-flag-payloads.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +`evaluate_flags()` now JSON-decodes payloads for locally-evaluated flags, the same way it already did for flags resolved remotely. Previously `get_flag_payload()` returned a parsed value (`{"copy": "new"}`) when the flag came back from `/flags` but the raw JSON string (`'{"copy": "new"}'`) when the poller evaluated it locally, so the payload's type depended on where the flag happened to resolve. The `$feature_flag_payload` property on `$feature_flag_called` events is decoded for locally-evaluated flags too. Payload strings that aren't valid JSON are still passed through unchanged. diff --git a/posthog/client.py b/posthog/client.py index 74a5d9eb..a9fa0ee9 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -244,6 +244,20 @@ def _parse_has_experiment(value: Any) -> Optional[bool]: return value if isinstance(value, bool) else None +def _parse_flag_payload(raw_payload: Any) -> Optional[Any]: + """Flag payloads are stored as JSON strings, both in the ``/flags`` response + metadata and in the local-evaluation flag definitions, so decode them before + handing them to callers. A string that isn't valid JSON is passed through as-is.""" + if isinstance(raw_payload, str): + if not raw_payload: + return None + try: + return json.loads(raw_payload) + except (json.JSONDecodeError, TypeError): + return raw_payload + return raw_payload + + def _metadata_has_experiment(metadata: Any) -> Optional[bool]: """Server-reported experiment linkage from flag metadata; ``None`` when absent (e.g. ``LegacyFlagMetadata``, which doesn't carry the field).""" @@ -3525,7 +3539,7 @@ def evaluate_flags( key=key, enabled=value is not False, variant=value if isinstance(value, str) else None, - payload=local_payloads.get(key), + payload=_parse_flag_payload(local_payloads.get(key)), id=flag_def.get("id"), # The local-evaluation flag definition does not carry a version field; # only the remote ``/flags`` response does via ``metadata.version``. @@ -3564,19 +3578,11 @@ def evaluate_flags( for key, detail in response.get("flags", {}).items(): if key in locally_evaluated_keys: continue - payload: Optional[Any] = None - raw_payload = ( + payload = _parse_flag_payload( detail.metadata.payload if isinstance(detail.metadata, FlagMetadata) else getattr(detail.metadata, "payload", None) ) - if isinstance(raw_payload, str) and raw_payload: - try: - payload = json.loads(raw_payload) - except (json.JSONDecodeError, TypeError): - payload = raw_payload - elif raw_payload is not None: - payload = raw_payload records[key] = _EvaluatedFlagRecord( key=key, enabled=detail.enabled, diff --git a/posthog/test/test_evaluate_flags.py b/posthog/test/test_evaluate_flags.py index dec72b46..485f175e 100644 --- a/posthog/test/test_evaluate_flags.py +++ b/posthog/test/test_evaluate_flags.py @@ -271,6 +271,85 @@ def test_empty_distinct_id_returns_empty_snapshot_without_events( self.assertEqual(len(feature_flag_called), 0) +class TestEvaluateFlagsLocalPayloads(unittest.TestCase): + """Locally-evaluated payloads must be decoded the same way remote ones are, + so a flag's payload type doesn't depend on where it happened to resolve.""" + + def setUp(self): + self.client = Client(FAKE_TEST_API_KEY, secret_key="test") + self.client.feature_flags = [ + { + "id": 1, + "name": "Checkout", + "key": "checkout", + "active": True, + "filters": { + "groups": [{"properties": [], "rollout_percentage": 100}], + "multivariate": { + "variants": [{"key": "blue", "rollout_percentage": 100}] + }, + "payloads": {"blue": '{"copy": "new"}'}, + }, + }, + { + "id": 2, + "name": "Beta UI", + "key": "beta-ui", + "active": True, + "filters": { + "groups": [{"properties": [], "rollout_percentage": 100}], + "payloads": {"true": '{"color": "green"}'}, + }, + }, + { + "id": 3, + "name": "Plain", + "key": "plain-payload", + "active": True, + "filters": { + "groups": [{"properties": [], "rollout_percentage": 100}], + "payloads": {"true": "not json"}, + }, + }, + ] + + def tearDown(self): + self.client.shutdown() + + @mock.patch("posthog.client.flags") + def test_local_payloads_are_parsed(self, patch_flags): + flags = self.client.evaluate_flags("user-1") + + self.assertEqual(flags.get_flag_payload("checkout"), {"copy": "new"}) + self.assertEqual(flags.get_flag_payload("beta-ui"), {"color": "green"}) + # /flags is not called because everything evaluated locally + self.assertEqual(patch_flags.call_count, 0) + + @mock.patch("posthog.client.flags") + def test_non_json_local_payload_is_passed_through(self, patch_flags): + flags = self.client.evaluate_flags("user-1") + + self.assertEqual(flags.get_flag_payload("plain-payload"), "not json") + self.assertEqual(patch_flags.call_count, 0) + + @mock.patch("posthog.client.flags") + @mock.patch.object(Client, "capture") + def test_flag_called_event_carries_parsed_local_payload( + self, patch_capture, patch_flags + ): + flags = self.client.evaluate_flags("user-1") + flags.get_flag("checkout") + + feature_flag_called = [ + c + for c in patch_capture.call_args_list + if c[0] and c[0][0] == "$feature_flag_called" + ] + self.assertEqual(len(feature_flag_called), 1) + properties = feature_flag_called[0][1]["properties"] + self.assertEqual(properties["$feature_flag_payload"], {"copy": "new"}) + + class TestEvaluateFlagsLocalDeviceBucketing(unittest.TestCase): def setUp(self): self.client = Client(FAKE_TEST_API_KEY)