fix(flags): return no payload for a failed flag record

.sampo/changesets/no-payload-for-failed-flag.md

@@ -0,0 +1,5 @@
+---
+pypi/posthog: patch
+---
+
+A flag record that the server marks `failed` no longer returns a payload, even when the record still carries `metadata.payload`. A failed flag reads as disabled, so `get_feature_flag_payload()`, `get_feature_flag_result().payload` and a snapshot's `get_flag_payload()` now return `None` for it, and its `$feature_flag_called` events leave out `$feature_flag_payload`. `get_all_flags_and_payloads()` and `get_feature_payloads()` already left it out. This applies to responses from both `/flags?v=4` and older servers.

posthog/test/fixtures/released_flags_7_82_1.json

@@ -990,6 +990,9 @@
             "$feature_flag_evaluated_at": 1800000000000,
             "$feature_flag_has_experiment": false,
             "$feature_flag_id": 700,
+            "$feature_flag_payload": {
+              "columns": 2
+            },
             "$feature_flag_reason": "Matched condition set 1",
             "$feature_flag_request_id": "60000000-0000-4000-8000-000000000000",
             "$feature_flag_response": false,
@@ -1005,6 +1008,9 @@
             "$feature_flag_evaluated_at": 1800000000000,
             "$feature_flag_has_experiment": false,
             "$feature_flag_id": 700,
+            "$feature_flag_payload": {
+              "columns": 2
+            },
             "$feature_flag_reason": "Matched condition set 1",
             "$feature_flag_request_id": "60000000-0000-4000-8000-000000000000",
             "$feature_flag_response": false,
@@ -1020,6 +1026,9 @@
             "$feature_flag_evaluated_at": 1800000000000,
             "$feature_flag_has_experiment": false,
             "$feature_flag_id": 700,
+            "$feature_flag_payload": {
+              "columns": 2
+            },
             "$feature_flag_reason": "Matched condition set 1",
             "$feature_flag_request_id": "60000000-0000-4000-8000-000000000000",
             "$feature_flag_response": false,
@@ -1232,6 +1241,9 @@
             "$feature_flag_evaluated_at": 1800000000000,
             "$feature_flag_has_experiment": false,
             "$feature_flag_id": 700,
+            "$feature_flag_payload": {
+              "columns": 2
+            },
             "$feature_flag_reason": "Matched condition set 1",
             "$feature_flag_request_id": "60000000-0000-4000-8000-000000000000",
             "$feature_flag_response": false,
@@ -1366,7 +1378,9 @@
       "get_feature_flag_payload:bool-off": null,
       "get_feature_flag_payload:bool-on": null,
       "get_feature_flag_payload:missing": null,
-      "get_feature_flag_payload:server-failed": null,
+      "get_feature_flag_payload:server-failed": {
+        "columns": 2
+      },
       "get_feature_flag_payload:sparse": null,
       "get_feature_flag_payload:v1-bool-payload": {
         "cta": "Try it",
@@ -1402,7 +1416,9 @@
       "get_feature_flag_result:server-failed": [
         false,
         null,
-        null,
+        {
+          "columns": 2
+        },
         "Matched condition set 1"
       ],
       "get_feature_flag_result:sparse": [
@@ -1559,7 +1575,7 @@
               "description": null,
               "has_experiment": false,
               "id": 700,
-              "payload": null,
+              "payload": "{\"columns\": 2}",
               "version": 3
             },
             "reason": {
@@ -1743,7 +1759,9 @@
       "snapshot.get_flag_payload:bool-off": null,
       "snapshot.get_flag_payload:bool-on": null,
       "snapshot.get_flag_payload:missing": null,
-      "snapshot.get_flag_payload:server-failed": null,
+      "snapshot.get_flag_payload:server-failed": {
+        "columns": 2
+      },
       "snapshot.get_flag_payload:sparse": null,
       "snapshot.get_flag_payload:v1-bool-payload": {
         "cta": "Try it",

posthog/test/released_flags_scenarios.py

@@ -76,7 +76,8 @@ def _flag(key: str, value: Any, **fields: Any) -> Dict[str, Any]:
     _flag("v2-number", 12.5, config_version=2),
     _flag("v2-object", {"layout": "compact", "n": [1, True]}, config_version=2),
     _flag("sparse", "compact", metadata={}, has_experiment=None, reason=None),
-    _flag("server-failed", "compact", failed=True),
+    # A failed record that still carries a payload.
+    _flag("server-failed", "compact", failed=True, payload='{"columns": 2}'),
 ]
 
 

posthog/test/test_flags_released_behaviour.py

@@ -38,6 +38,26 @@ def test_every_legacy_getter_and_event_matches_7_82_1(results):
     assert (sparse["id"], sparse["version"]) == (0, 0)
     sparse["id"] = sparse["version"] = None
 
+    # A failed record reads as disabled, so it has no payload, even when the record
+    # carries one. 7.82.1 returned that payload from the payload reads and sent it on
+    # the flag's $feature_flag_called events.
+    failed = expected["get_flags_decision"]["flags"]["server-failed"]["metadata"]
+    assert failed["payload"] == '{"columns": 2}'
+    failed["payload"] = None
+    for read in ["get_feature_flag_payload", "snapshot.get_flag_payload"]:
+        assert expected[f"{read}:server-failed"] == {"columns": 2}
+        expected[f"{read}:server-failed"] = None
+    assert expected["get_feature_flag_result:server-failed"][2] == {"columns": 2}
+    expected["get_feature_flag_result:server-failed"][2] = None
+    failed_events = [
+        properties
+        for _, properties in expected["events"]
+        if properties.get("$feature_flag") == "server-failed"
+    ]
+    assert failed_events
+    for properties in failed_events:
+        assert properties.pop("$feature_flag_payload") == {"columns": 2}
+
     assert results["getters"] == expected
 
 
@@ -71,5 +91,14 @@ def test_fallback_cache_histories_match_7_82_1(results, name):
     # An unreadable record now fails only itself, but a single-flag read of it
     # still reports what 7.82.1 reported when the whole response failed: an
     # unknown_error, no request details, and the cached value.
-    expected = RELEASED["cache histories"][name]
+    expected = copy.deepcopy(RELEASED["cache histories"][name])
+    if name == "good, server failed, timeout":
+        # A failed record has no payload, even when the record carries one, so
+        # neither its read nor the cached result read after the timeout returns
+        # one, and their events leave it out. 7.82.1 returned it from both.
+        for result in expected["results"][1:]:
+            assert result[2] == {"theme": "dark"}
+            result[2] = None
+        for _, properties in expected["events"][1:]:
+            assert properties.pop("$feature_flag_payload") == {"theme": "dark"}
     assert results["cache histories"][name] == expected

posthog/test/test_flags_v4_response.py

@@ -746,6 +746,109 @@ def test_a_config_version_1_payload_is_kept():
     assert flags["string-flag"].metadata.payload == '{"columns": 2}'
 
 
+def _failed_with_payload(shape: str) -> Dict[str, Any]:
+    """A failed record that still carries a payload, in the given response shape."""
+    if shape == "v4":
+        # A producer-valid failed config version 1 record with a payload.
+        [case] = [c for c in READERS["cases"] if c["id"] == "readers.v1_failed"]
+        response = _build(READERS, case)
+        record = response["flags"]["garden-layout"]
+        assert record["failed"] is True
+        assert record["metadata"]["payload"] == '{"columns":2}'
+        return response
+    record = _older_record(enabled=False, variant=None, failed=True)
+    record["metadata"]["payload"] = '{"columns":2}'
+    return {"flags": {"garden-layout": record}, "requestId": "r1"}
+
+
+@pytest.mark.parametrize("shape", ["v4", "older"])
+def test_a_failed_record_has_no_payload_on_any_read(shape, client):
+    # A failed flag reads as disabled, so a payload on it would surprise the caller.
+    response = _failed_with_payload(shape)
+    [flag] = normalize_flags_response(copy.deepcopy(response))["flags"].values()
+    assert flag._failed is True
+    assert flag.metadata.payload is None
+
+    with mock.patch("posthog.client.flags", return_value=copy.deepcopy(response)):
+        result = client.get_feature_flag_result("garden-layout", DISTINCT_ID)
+        assert result is not None and result.payload is None
+        assert client.get_feature_flag_payload("garden-layout", DISTINCT_ID) is None
+        assert (
+            client.get_feature_flag_payload(
+                "garden-layout", "user-2", send_feature_flag_events=True
+            )
+            is None
+        )
+        payloads = client.get_all_flags_and_payloads(DISTINCT_ID)["featureFlagPayloads"]
+        assert "garden-layout" not in (payloads or {})
+        assert "garden-layout" not in (client.get_feature_payloads(DISTINCT_ID) or {})
+        snapshot = client.evaluate_flags(DISTINCT_ID)
+        assert snapshot.get_flag_payload("garden-layout") is None
+
+
+def _disabled_with_payload(shape: str, failed: Any) -> Dict[str, Any]:
+    """A disabled record that is not failed and carries a payload.
+
+    ``failed`` is the value of the record's ``failed`` field, or ``_ABSENT`` to
+    leave the field out.
+    """
+    if shape == "v4":
+        [case] = [c for c in READERS["cases"] if c["id"] == "readers.v1_at_v4"]
+        response = _build(READERS, case)
+        record = response["flags"]["garden-layout"]
+        record["value"] = False
+        record["reason"] = {"code": "no_condition_match", "condition_index": None}
+        del record["metadata"]["variant_key"]
+        assert record["metadata"]["payload"] == '{"columns":2}'
+    else:
+        record = _older_record(enabled=False, variant=None)
+        record["metadata"]["payload"] = '{"columns":2}'
+        response = {"flags": {"garden-layout": record}, "requestId": "r1"}
+    if failed is _ABSENT:
+        record.pop("failed", None)
+    else:
+        record["failed"] = failed
+    return response
+
+
+_ABSENT = object()
+
+
+# A null ``failed`` is a null optional field, which both parsers ignore, so it is a
+# valid control in both shapes.
+@pytest.mark.parametrize(
+    "failed", [False, None, _ABSENT], ids=["false", "null", "absent"]
+)
+@pytest.mark.parametrize("shape", ["v4", "older"])
+def test_a_disabled_record_that_is_not_failed_keeps_its_payload(shape, failed, client):
+    # The control for the test above: only a failed record loses its payload.
+    response = _disabled_with_payload(shape, failed)
+    [flag] = normalize_flags_response(copy.deepcopy(response))["flags"].values()
+    assert flag._failed is False
+    assert flag.metadata.payload == '{"columns":2}'
+
+    with mock.patch("posthog.client.flags", return_value=copy.deepcopy(response)):
+        result = client.get_feature_flag_result("garden-layout", DISTINCT_ID)
+        assert result is not None and result.payload == {"columns": 2}
+        assert client.get_feature_flag_payload("garden-layout", DISTINCT_ID) == {
+            "columns": 2
+        }
+        snapshot = client.evaluate_flags(DISTINCT_ID)
+        assert snapshot.get_flag_payload("garden-layout") == {"columns": 2}
+
+
+async def test_a_failed_record_has_no_payload_on_the_async_client():
+    with mock.patch(
+        "posthog.async_client._async_flags",
+        new=mock.AsyncMock(return_value=_failed_with_payload("v4")),
+    ):
+        client = AsyncPosthog("project-key", send=False)
+        snapshot = await client.evaluate_flags(DISTINCT_ID)
+        await client.shutdown()
+
+    assert snapshot.get_flag_payload("garden-layout") is None
+
+
 def test_feature_flag_value_defaults_to_the_legacy_value():
     assert FeatureFlag.from_value_and_payload("a", "control", None)._value == "control"
     assert FeatureFlag.from_value_and_payload("a", True, None)._value is True

posthog/types.py

@@ -389,13 +389,15 @@ def _parse_typed_record(cls: Type[_F], key: str, resp: Any) -> _F:
         if raw_metadata is None:
             metadata = LegacyFlagMetadata(payload=encoded_payload)
         else:
+            # A number or object value is its own payload; a payload the record also
+            # carries is ignored. A failed record reads as disabled, so it has no
+            # payload, even when the record carries one.
+            payload = encoded_payload
+            if payload is None and not failed:
+                payload = raw_metadata.get("payload")
             metadata = FlagMetadata(
                 id=_read_int(raw_metadata.get("id"), "metadata.id"),
-                # A number or object value is its own payload; a payload the record
-                # also carries is ignored.
-                payload=encoded_payload
-                if encoded_payload is not None
-                else raw_metadata.get("payload"),
+                payload=payload,
                 version=_read_int(raw_metadata.get("version"), "metadata.version"),
                 description=_read_typed(
                     raw_metadata.get("description", ""), "metadata.description", str
@@ -469,6 +471,10 @@ def _parse_older_record(cls: Type[_F], key: str, resp: Dict[str, Any]) -> _F:
     except _MalformedRecord as error:
         return _malformed_record(cls, key, str(error))
 
+    failed = resp.get("failed") is True
+    if failed and raw_metadata:
+        # A failed record has no payload, even when the record carries one.
+        raw_metadata = {**raw_metadata, "payload": None}
     flag = cls(
         key=key,
         # An older server always sends ``enabled``; a record without it keeps
@@ -478,7 +484,7 @@ def _parse_older_record(cls: Type[_F], key: str, resp: Dict[str, Any]) -> _F:
         reason=FlagReason.from_json(raw_reason),
         metadata=FlagMetadata.from_json(raw_metadata),
     )
-    return _set_private_fields(flag, _failed=resp.get("failed") is True)
+    return _set_private_fields(flag, _failed=failed)
 
 
 def _read_record_key(resp: Dict[str, Any]) -> None: