Repository navigation
chore!: return nil for malformed feature flag payloads - #903
Conversation
|
[Medium risk] Changes how feature flag payloads are decoded and validated. The PR is not safe to merge without handling valid bootstrap strings persisted by earlier installs. Reviews (1) · Last reviewed commit: "chore!: return nil for malformed feature..." |
posthog-ios Compliance ReportDate: 2026-10-07 19:01:06 UTC
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 2870ms |
| Format Validation.Event Has Uuid | ✅ | 2751ms |
| Format Validation.Event Has Lib Properties | ✅ | 2653ms |
| Format Validation.Distinct Id Is String | ✅ | 3023ms |
| Format Validation.Token Is Present | ✅ | 2754ms |
| Format Validation.Custom Properties Preserved | ✅ | 2706ms |
| Format Validation.Event Has Timestamp | ✅ | 2765ms |
| Retry Behavior.Retries On 503 | ✅ | 11232ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 4647ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 5123ms |
| Retry Behavior.Respects Retry After Header | ✅ | 7749ms |
| Retry Behavior.Implements Backoff | ✅ | 21226ms |
| Retry Behavior.Retries On 500 | ✅ | 8800ms |
| Retry Behavior.Retries On 502 | ✅ | 8828ms |
| Retry Behavior.Retries On 504 | ✅ | 8724ms |
| Retry Behavior.Max Retries Respected | ❌ | 21276ms |
| Deduplication.Generates Unique Uuids | ✅ | 3229ms |
| Deduplication.Preserves Uuid On Retry | ✅ | 9305ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ✅ | 16219ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ✅ | 8067ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 2808ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 2884ms |
| Compression.Sends Gzip When Enabled | ✅ | 2770ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 2777ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 285ms |
| Batch Format.Multiple Events Batched Together | ✅ | 2856ms |
| Error Handling.Does Not Retry On 403 | ✅ | 4797ms |
| Error Handling.Does Not Retry On 413 | ✅ | 4724ms |
| Error Handling.Retries On 408 | ✅ | 9310ms |
Failures
retry_behavior.max_retries_respected
Expected 4 requests, got 6
Feature_Flags Tests
✅ 16/16 tests passed
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 2808ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 2652ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 2785ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 2687ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 2752ms |
| Request Payload.Groups Round Trip | ✅ | 2665ms |
| Request Payload.Groups Default To Empty Object | ✅ | 2806ms |
| Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It | ✅ | 2651ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 2651ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ✅ | 2734ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 2662ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 60ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 2768ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 5464ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ✅ | 2666ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ✅ | 2995ms |
61dcd05 to
f8cdf35
Compare
e1cc8cf to
5b6c50e
Compare
Follow the sdk-specs payload decoding contract: a serialized payload that fails to decode (malformed, empty or whitespace-only) now reads as nil and is logged, instead of leaking the raw string. The flag's value, enabled state and sibling payloads are unaffected. Bootstrapped payloads are already decoded, so they are now serialized when seeded into the payload cache. Reads decode each payload exactly once, so a bootstrapped "123" or "true" string stays a String instead of becoming a number or Bool. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
f8cdf35 to
f8f3a4e
Compare
dustinbyrne
left a comment
There was a problem hiding this comment.
No actionable defect found in this diff; I recommend approval of the payload change. Valid string and object payloads remain supported, and the malformed-payload behavior matches sdk-specs and the backend's JSON-encoded payload convention. The added regression tests passed on macOS and iOS.
Consider separately triaging the failed iOS simulator check before merging: the unchanged ignored-exception test at PostHogErrorTrackingIgnoredTypesTest.swift:169 captured an unexpected $exception event. It passed on the exact base; I did not establish a causal connection to this PR or confirm a flake. The compliance retry-count failure is also present on the exact base despite the green aggregate check.
AI-assisted review.
dustinbyrne
left a comment
There was a problem hiding this comment.
Approved: no actionable defect found in the payload changes. Valid string and object payloads remain supported, and malformed serialized payloads now follow the canonical decoding contract. The added regression tests passed on macOS and iOS.
The existing iOS simulator failure still needs separate disposition before merging, as described in my preceding review. Its failing error-tracking path is unchanged here; I have not confirmed a flake or established a causal connection to this PR.
AI-assisted review.
…pture test storage.reset() keeps the event queue, and getBatchedEvents returns once the mock receives a batch, before the SDK deletes the sent file. The previous test's $exception could survive on disk and be resent by the next SDK, failing the "drops ignored types" assertion on the iOS simulator job. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Require posthog-ios 4.x. - Adapt to the reloadFeatureFlags(_:) callback that now passes a PostHogFeatureFlagsLoaded (PostHog/posthog-ios#913). - Note the payload decoding changes from PostHog/posthog-ios#903. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Require posthog-ios 4.x. - Adapt to the reloadFeatureFlags(_:) callback that now passes a PostHogFeatureFlagsLoaded (PostHog/posthog-ios#913). - Note the payload decoding changes from PostHog/posthog-ios#903. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
* chore!: return nil for malformed feature flag payloads Follow the sdk-specs payload decoding contract: a serialized payload that fails to decode (malformed, empty or whitespace-only) now reads as nil and is logged, instead of leaking the raw string. The flag's value, enabled state and sibling payloads are unaffected. Bootstrapped payloads are already decoded, so they are now serialized when seeded into the payload cache. Reads decode each payload exactly once, so a bootstrapped "123" or "true" string stays a String instead of becoming a number or Bool. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test: cover bootstrapped string payloads read back after a restart Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test: clear leftover queue files before each ignoredExceptionTypes capture test storage.reset() keeps the event queue, and getBatchedEvents returns once the mock receives a batch, before the SDK deletes the sent file. The previous test's $exception could survive on disk and be resent by the next SDK, failing the "drops ignored types" assertion on the iOS simulator job. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
💡 Motivation and Context
Part of the 4.0 stack, stacked on #873. It brings iOS in line with the sdk-specs payload decoding contract (
get-feature-flag-payload, "Malformed serialized payloads return no payload", plus the matchingget-feature-flag-resultandget-feature-flags-and-payloadsrequirements).{broken), empty or whitespace-onlynil, logged with the flag key"hello","",false,0, objects, arrays)nullNSNullNSNull(unchanged; the spec keeps JSON null)"123"/"true"123/trueStringPostHogRemoteConfigare now one helper, used bygetFeatureFlagResult(_:)andgetAllFeatureFlags()./flagspayloads, which breaks the spec's "don't decode an already-decoded string again" rule. They're now serialized when seeded into the cache, so every cached payload is decoded exactly once. Bootstrap values already pass throughsanitizeDictionary, so serialization can't hit a non-JSON value.Public API change: no signature changes; behavior change only (
majorchangeset).Edge case: bootstrap is re-seeded on every launch that passes it, and any successful
/flagsresponse replaces the cache. So the only affected case is an app whose last 3.x launch seeded a bootstrap string payload, never got a/flagsresponse after that, and stops passing bootstrap in 4.x. That payload reads asnil(if it isn't valid JSON on its own) until the first/flagsresponse. Non-string bootstrap payloads (objects, arrays, numbers) are unaffected. Telling legacy entries apart would need a storage format marker, which isn't worth it for this case.Wrappers:
getFeatureFlagResultpayloads through unchanged, so no code change. Once Flutter v6 adopts iOS 4.0, a malformed payload reads asnullon iOS while posthog-android still returns the raw string (normalizePayloadskeeps the original value on a parse failure). Android needs its own change to meet the spec.💚 How did you test it?
PostHogFeatureFlagsTest: malformed / empty / whitespace payloads returnnilwhile the flag and a sibling payload are preserved; valid JSON keeps its type; bootstrapped"hello","123","true"stay strings. Before the fix they failed with 11 issues for exactly those cases."hello","123"or"true"still reads back as aStringfrom a new instance with the same storage and no bootstrap. It fails if bootstrap payloads are cached without serializing.swift test --filter PostHogFeatureFlagsTestpasses.make test(macOS), after rebasing on chore!: iOS Major Release 4.0 #873: 965 tests; the only failures are the twoPostHogStorageTestapp-group tests, which fail the same way on the base commit on this machine.make buildSdk(6 platforms) and the public API check pass.make lint: clean apart from existing warnings.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset fileThe changeset is
majorand was written by hand.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
"hello"intonil.🤖 Generated with Claude Code