diff --git a/.sampo/changesets/steadfast-baroness-vellamo.md b/.sampo/changesets/steadfast-baroness-vellamo.md new file mode 100644 index 000000000..71ce26f28 --- /dev/null +++ b/.sampo/changesets/steadfast-baroness-vellamo.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +Return None for malformed JSON feature flag payloads instead of returning the raw string or raising a JSONDecodeError. diff --git a/posthog/client.py b/posthog/client.py index 59ccb9dca..159a0e266 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -1,6 +1,5 @@ import atexit import inspect -import json import logging import os import sys @@ -93,7 +92,8 @@ remote_config, reset_sessions, ) -from posthog.types import ( +from .types import ( + _parse_flag_payload, FeatureFlag, FeatureFlagError, FeatureFlagResult, @@ -326,20 +326,6 @@ 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).""" diff --git a/posthog/test/test_evaluate_flags.py b/posthog/test/test_evaluate_flags.py index 31d332661..32a63ab27 100644 --- a/posthog/test/test_evaluate_flags.py +++ b/posthog/test/test_evaluate_flags.py @@ -404,10 +404,10 @@ def test_local_payloads_are_parsed(self, patch_flags): self.assertEqual(patch_flags.call_count, 0) @mock.patch("posthog.client.flags") - def test_non_json_local_payload_is_passed_through(self, patch_flags): + def test_non_json_local_payload_returns_none(self, patch_flags): flags = self.client.evaluate_flags("user-1") - self.assertEqual(flags.get_flag_payload("plain-payload"), "not json") + self.assertIsNone(flags.get_flag_payload("plain-payload")) self.assertEqual(patch_flags.call_count, 0) @mock.patch("posthog.client.flags") diff --git a/posthog/test/test_flag_payload_parsing.py b/posthog/test/test_flag_payload_parsing.py new file mode 100644 index 000000000..678694c1c --- /dev/null +++ b/posthog/test/test_flag_payload_parsing.py @@ -0,0 +1,69 @@ +from unittest.mock import patch + +import pytest + +from posthog.client import Client + + +@pytest.mark.parametrize("local", [True, False]) +@pytest.mark.parametrize("legacy", [True, False]) +@pytest.mark.parametrize( + "raw, expected", + [ + ('{"broken":', None), + ("not json", None), + (" ", None), + ("", None), + ("[1, 2]", [1, 2]), + ('{"ok": true}', {"ok": True}), + ('"text"', "text"), + ("false", False), + ("0", 0), + ("null", None), + ({"decoded": True}, {"decoded": True}), + (None, None), + ], +) +def test_payload_parsing(local, legacy, raw, expected): + client = Client("test-key", send=False) + if local: + client.feature_flags = [ + { + "id": 1, + "key": "test-flag", + "active": True, + "filters": { + "groups": [{"properties": [], "rollout_percentage": 100}], + "payloads": {"true": raw}, + }, + } + ] + response = { + "flags": { + "test-flag": { + "key": "test-flag", + "enabled": True, + "variant": None, + "reason": {"code": "condition_match", "description": "Matched"}, + "metadata": {"id": 1, "version": 1, "payload": raw}, + } + } + } + try: + with ( + patch.object(client, "load_feature_flags"), + patch("posthog.client.flags", return_value=response) as request, + ): + if legacy: + with pytest.warns(DeprecationWarning): + result = client.get_feature_flag_payload( + "test-flag", "user", only_evaluate_locally=local + ) + else: + result = client.evaluate_flags( + "user", only_evaluate_locally=local + ).get_flag_payload("test-flag") + assert result == expected + assert request.call_count == (0 if local else 1) + finally: + client.shutdown() diff --git a/posthog/types.py b/posthog/types.py index 8e8d7a637..0f7ed131e 100644 --- a/posthog/types.py +++ b/posthog/types.py @@ -4,6 +4,16 @@ FlagValue = Union[bool, str] + +def _parse_flag_payload(raw_payload: Any) -> Optional[Any]: + if isinstance(raw_payload, str): + try: + return json.loads(raw_payload) + except json.JSONDecodeError: + return None + return raw_payload + + # Type alias for the before_send callback function # Takes an event dictionary and returns the modified event or None to drop it BeforeSendCallback = Callable[[dict[str, Any]], Optional[dict[str, Any]]] @@ -231,9 +241,7 @@ def from_value_and_payload( key=key, enabled=enabled, variant=variant, - payload=json.loads(payload) - if isinstance(payload, str) and payload - else payload, + payload=_parse_flag_payload(payload), reason=None, ) @@ -271,12 +279,7 @@ def from_flag_details( key=details.key, enabled=enabled, variant=variant, - payload=( - json.loads(details.metadata.payload) - if isinstance(details.metadata.payload, str) - and details.metadata.payload - else details.metadata.payload - ), + payload=_parse_flag_payload(details.metadata.payload), reason=details.reason.description if details.reason else None, )