Skip to content

fix: send unknown entitlement details as null, not as missing - #517

Merged
yusuftor merged 5 commits into
developfrom
fix/entitlement-unknown-fields
Sep 7, 2026
Merged

fix: send unknown entitlement details as null, not as missing#517
yusuftor merged 5 commits into
developfrom
fix/entitlement-unknown-fields

Conversation

@yusuftor

@yusuftor yusuftor commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

What

An entitlement supplied by a Purchase Controller carries no renewal metadata — willRenew, isLifetime, state, offerType are all nil. Entitlement.encode(to:) used encodeIfPresent, which drops those keys entirely.

Audience filters give a missing key the type default, so willRenew read as false and a bare active entitlement matched an audience like "active and will not renew":

{ "identifier": "unlimited_access", "isActive": true }

On app 4531 / campaign 37465 that was all 181 matches in a 24h window; only 10 of those users had actually turned off renewal, and 53 were explicitly renewing.

How

Unknown detail fields are written as explicit nulls, which filters treat as unknown rather than as a value. Verified against the shipped Superscript 1.0.15 — no evaluator change needed:

entitlement willRenew == false
willRenew unknown no match (was: match)
willRenew: false match
willRenew: true no match

willRenew == null now answers "is this known?", which was previously impossible to ask.

Two hops had to change, and either alone is broken:

  1. Entitlement.encode(to:) writes the null.
  2. toPassableValue(from:) gained case is NSNull: return .null. Without it the null fell through default:JSON(NSNull()) → the trailing reduce → an empty map, which can't be compared to anything (Map can not be compared to Int(10)). PassableValue.null already existed but was only ever produced when decoding results coming back.

Scoped to audience filters only

Entitlement.encode(to:) is the SDK's only entitlement encoder, so nulling unconditionally would also have changed the enrichment request body, the paywall template variables, the session attributes and the public getDeviceAttributes() — six new nulls per bare entitlement on surfaces that never needed them.

The nulls are now opt-in via a reportsUnknownFieldsAsNull userInfo key, threaded from exactly one call site:

makeAudienceFilterAttributes → getDeviceAttributes(reportingUnknownFieldsAsNull: true)
                             → getTemplateDevice → DeviceTemplate.toDictionary(encoder:)
                             → Entitlement.encode → encodeNil

Every other caller uses a default JSONEncoder and produces byte-identical output to develop. Two tests pin both halves, including that userInfo survives into nested entitlements — the flag is set on the whole template but consumed two levels down.

For the record, the enrichment endpoint would have accepted the nulls anyway: apps/enrichment-api/src/server.ts runs no validation on the device payload, and prepareRequest destructures eight named fields and never reads entitlement data. The scoping is about not changing shapes that didn't need to change.

Deliberate exception: dates

startsAt / renewedAt / expiresAt are never nulled. Filters compare them with < and >, and a null on either side of an ordering comparison makes the whole filter evaluate to null — which then swallows unrelated clauses (x < 10 || true is null, not true). A test pins this so it isn't "tidied up" later.

Verified, not assumed: the string-valued fields

Four of the six nulled fields encode as strings. Checked against Superscript 1.0.15, comparing present-null to the old absent-key behaviour:

  • Equality is identical either waystore == "APP_STORE" is false, != is true, state in [...] is false, and || true short-circuits correctly.
  • String functions error identically either way. startsWith, matches and size return Err for both null and absent, because the member rewrite already hands them null when the key is missing. So a filter calling a string function on an unknown latestProductId already kills the whole audience via the .failure -> noMatch path — this PR neither causes nor fixes that. Worth its own issue.

The one genuine flip is has(e.willRenew), false → true. That's the mechanism of the fix rather than a side effect, and it's pinned as a test.

Scope

Fixes the false-positive half of the issue. Not addressed here: a complete atomic entitlement snapshot from Purchase Controllers, the Expo/RN field preservation and RevenueCat mapping, audience diagnostics naming the evaluation source, and a server-authoritative entitlement option.

The Android side is Superwall-Android#459 — a different bug (a JSON null became the string "null"). The incident bug can't occur there because Android's DeviceTemplate exposes only entitlement ids and types, never the detail fields.

Testing

  • New EntitlementUnknownFieldsTests — 13 tests: both hops separately, the incident's exact filter end-to-end through the real Superscript binary (bare / explicit-false / explicit-true / == null / has()), the four string-valued fields, nested userInfo propagation, and that every non-filter consumer still omits the keys.
  • Full suite: 989 tests, 0 failures.
  • swiftlint: clean on all changed files.

Checklist

  • All unit tests pass.
  • All UI tests pass.
  • Demo project builds and runs on iOS.
  • Demo project builds and runs on Mac Catalyst.
  • Demo project builds and runs on visionOS.
  • I added/updated tests or detailed why my change isn't tested.
  • I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
  • I have run swiftlint in the main directory and fixed any issues.
  • I have updated the SDK documentation as well as the online docs.
  • I have reviewed the contributing guide

🤖 Generated with Claude Code

An entitlement supplied by a Purchase Controller carries no renewal
metadata, and `encodeIfPresent` dropped those keys entirely. Audience
filters give a missing key the type default, so `willRenew` read as
`false` and an active subscriber matched a "will not renew" audience.
On one app that was every one of 181 matches over a day, while only 10
had actually cancelled.

The keys are now written as explicit nulls, which filters treat as
unknown: a bare entitlement no longer matches `willRenew == false`, an
explicit `false` still does, and `willRenew == null` can ask whether the
detail is known at all.

Two hops had to change for that to arrive. `Entitlement.encode(to:)`
writes the null, and `toPassableValue` maps `NSNull` onto
`PassableValue.null` - without that the null fell through to the default
branch and reached the filter as an empty map, which compares to nothing.

Dates stay omitted rather than nulled: filters compare them with `<` and
`>`, and a null on either side of an ordering comparison makes the whole
filter evaluate to null, taking unrelated clauses with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The equality-vs-ordering rationale is sound and well tested, but four of the six newly-nulled fields encode as JSON strings, and a null string has the same "no overload" failure mode as a null in an ordering comparison. Worth verifying against Superscript 1.0.15 before this ships to live campaigns.

Reviewed changes — full diff of the single commit 8a29ef0 (6 files) against develop, plus the surrounding entitlement encoding, audience-filter evaluation, and device-attribute plumbing.

  • Explicit nulls for unknown entitlement detailslatestProductId, store, isLifetime, willRenew, state and offerType move from encodeIfPresent to a new encodeNilOrValue, so a Purchase Controller's bare entitlement no longer reads as willRenew == false. Dates stay omitted on purpose.
  • NSNullPassableValue.null in toPassableValue(from:), without which the null fell through default:JSON(NSNull()) → the trailing reduce → an empty map. I traced this against SwiftyJSON.swift:237-238 and 306-320 and confirm the pre-change result was .map([:]), exactly as described.
  • New KeyedEncodingContainer helper encapsulating encodeNil vs encode.
  • New EntitlementUnknownFieldsTests — 9 tests, 5 of which genuinely fail if the production changes are reverted; the other 4 are deliberate controls pinning the match matrix.
  • CHANGELOG + pbxproj — the entry correctly lands in the already-staged 4.17.0 section (develop is 4.17.0, master is 4.16.3, so no bump is due), and all four pbxproj registrations are present for both new files.

Round-tripping is safe: Entitlement.init(from:) uses decodeIfPresent throughout, which treats absent-key and present-null identically, so cached LatestCustomerInfo / LatestDeviceCustomerInfo / LatestRedeemResponse data stays compatible in both directions, and V3Migrator never goes through encode(to:) at all.

ℹ️ The justification is scoped to audience filters, but Entitlement.encode(to:) is the SDK's only entitlement encoder

The same encoded shape also leaves the device in the enrichment request body (DeviceHelper.swift:932-934 via DeviceTemplate.swift:54-55), in the customerInfo_didChange event parameters that reach host-app delegates and third-party analytics (TrackableSuperwallEvent.swift:365-395), and out of the public Superwall.getDeviceAttributes() (Superwall.swift:311-314), where a key that used to be absent is now an NSNull. Nothing in this repo distinguishes absent from present-null, so the SDK itself is fine — the question is whether the backend and the wrapper SDKs are.

Technical details
# Confirm present-null is tolerated outside the filter evaluator

## Affected sites
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:932-934``EnrichmentRequest(device: JSON(deviceAttributes))` now carries `"willRenew":null` inside `device.customerInfo.entitlements[]`. `SwiftyJSON.swift:1340-1345` re-serialises `NSNull` as a JSON null, so it reaches the wire.
- `Sources/SuperwallKit/Analytics/Internal Tracking/Trackable Events/TrackableSuperwallEvent.swift:365-395``EntitlementsSnapshot` is encoded to a JSON *string* and returned as `from`/`to`. `TrackingLogic.processParameters` copies these into both `delegateParams` and `audienceFilterParams`, so they reach `handleSuperwallEvent`, any third-party analytics the host app forwards to, and the placement queue. This fires on every entitlement change.
- `Sources/SuperwallKit/Superwall.swift:311-314``public func getDeviceAttributes()`. A Swift host app doing `dict["willRenew"] == nil` used to get `true` and now gets `false`.
- `Sources/SuperwallKit/Misc/Extensions/Dictionary/Dictionary+Filter.swift:11``removingNSNullValues()` exists but a repo-wide grep finds only its own definition. Nothing strips these nulls on any path.

## Required outcome
- Confirmation that Superwall's enrichment and event ingestion treat a present-null field the same as an absent one (in particular that a null `store` or `state` isn't rejected as an invalid enum).
- Confirmation that the RN / Expo / Flutter wrappers don't do `hasOwnProperty`-style presence checks on these fields. The PR body notes Expo/RN field preservation is out of scope, so this may already be tracked.

## Open questions for the human
- Should the CHANGELOG entry mention the `getDeviceAttributes()` output-shape change, or is that surface considered internal enough to skip?

ℹ️ Nitpicks

  • encodeNilOrValue is behaviourally redundant with the standard library: try container.encode(willRenew, forKey: .willRenew) on a typed optional binds T = Optional<Bool>, and Optional's Encodable conformance calls encodeNil() for .none, so JSONEncoder already writes an explicit null (confirmed against the stdlib Codable.swift source and empirically). Keeping the helper is still defensible purely for the intent it documents at the call site — worth a line in its doc comment saying so, since "the stdlib already does this" is the first thing a future reader will notice.
  • The filter tests assert byte-exact Superscript output (#"{"Ok":{"type":"bool","value":false}}"#, test file lines 115, 122, 129, 140). EvaluationResult already decodes that, and CELEvaluatorTests asserts on the decoded outcome — decoding here too would survive a field-order or wrapper change in a future Superscript bump.
  • The evaluate(_:entitlement:) helper rebuilds CELEvaluator.evaluateExpression's pipeline by hand but passes computed: [:], device: [:] where production passes ComputedPropertyRequestType.allCases (CELEvaluator.swift:57-66), so it doesn't exercise the real consumer path and can drift from it.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread Sources/SuperwallKit/StoreKit/Products/StoreProduct/Entitlement.swift Outdated
Review on #517 asked whether the four string-encoded fields behave like
the boolean one, since a null string has no overload for `startsWith`,
`matches` or `size`. Checked against Superscript 1.0.15: equality against
a null string is a plain no-match, identical to the old absent-key
result, and the string functions error the same way whether the key is
absent or null - the member rewrite already hands them null either way.
So the change doesn't move that behaviour. Pinned as tests.

Also pins `has(e.willRenew)` flipping to true, which is the mechanism of
the fix rather than a side effect.

The helper now shares one dependency container and resets its storage
like `CELEvaluatorTests` does, passes the same computed/device maps as
`CELEvaluator`, and decodes the result instead of matching the raw JSON.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the delta from 8a29ef0 to 1647e0e, which is test-only plus one doc-comment addition; no production code changed in this commit.

  • String-field null hazard empirically closed — four parameterized filter cases (store, state, latestProductId, isLifetime) now run through the real Superscript binary and confirm equality against a present-null is a plain no-match. Worth noting these are controls rather than regression guards: an absent key gives the type default ("" / false) and a present-null gives CEL heterogeneous equality, and both land on false. They still earn their keep, because the helper's guard calls Issue.record when the result isn't .success(.bool), so an evaluator error fails the test instead of being absorbed by == false.
  • has(e.willRenew) pinned as the mechanism, not a side effect — the falsetrue flip is now a test. This one genuinely fails on revert: per the CEL spec, has() on a map is pure key existence, so it only reads true once the key is written.
  • Test harness matches production — hoisted to one static let DependencyContainer for the suite with storage.reset(), matching CELEvaluatorTests, and computed/device now carry ComputedPropertyRequestType.allCases exactly as CELEvaluator.swift:57-66 does. Dictionary(uniqueKeysWithValues:) can't trap here — all 10 description values are distinct.
  • Assertions decode instead of string-matchingevaluate(_:entitlement:) returns Bool off a decoded EvaluationResult, so a future Superscript wrapper reshape won't read as a behaviour change.
  • Helper doc records the stdlib redundancyencodeNilOrValue's comment now says plainly that encode(_:forKey:) on an optional already writes a null, and that the helper exists so the encode / encodeIfPresent distinction doesn't get "tidied up" back into the bug.

Both inline threads from the prior review were answered and resolved by the author, and I agree with the resolutions: the string-function failure mode is real but pre-existing and unchanged by this PR, and the harness now follows the sibling suite's pattern.

ℹ️ Two items from the prior review are still open and neither has a line to anchor to

The string-function investigation turned up a genuine pre-existing bug — a filter calling startsWith / matches / size on an unknown entitlement field errors, and CELEvaluator.swift:103-109 converts that into a noMatch for the whole audience. The comparison table shows it behaves identically before and after this change, so it correctly isn't this PR's problem, but it also isn't tracked anywhere yet. Separately, the downstream-consumer question is unanswered: Entitlement.encode(to:) is the SDK's only entitlement encoder, so present-null now also reaches the enrichment endpoint, customerInfo_didChange params, and getDeviceAttributes().

Technical details
# Carry the two off-repo items somewhere durable before merge

## Affected sites
- `Sources/SuperwallKit/Paywall/Presentation/Audience Logic/Expression Evaluator/CELEvaluator.swift:103-109` — any `.failure` becomes a whole-audience `noMatch`, so one string function on one unknown field silently disqualifies every user from that audience. Pre-existing; the author's 1647e0e table confirms present-null and absent-key produce the same error.
- `Sources/SuperwallKit/Network/Device Helper/DeviceHelper.swift:932-934`, `Sources/SuperwallKit/Analytics/Internal Tracking/Trackable Events/TrackableSuperwallEvent.swift:365-395`, `Sources/SuperwallKit/Superwall.swift:311-314` — the three non-filter consumers of the same encoded shape.

## Required outcome
- The string-function failure mode exists as a tracked issue rather than only as a PR comment, since nothing in this PR fixes it and the comment thread is now resolved.
- Confirmation that Superwall's ingestion and the RN / Expo / Flutter wrappers treat a present-null field the same as an absent one — in particular that a null `store` or `state` isn't rejected as an invalid enum, and that no wrapper does a `hasOwnProperty`-style presence check.

## Open questions for the human
- Is `getDeviceAttributes()` considered public enough that its output-shape change (a key that used to be absent is now `NSNull`) belongs in the CHANGELOG entry?

Pullfrog  | View workflow run | Using Claude Opus𝕏

Writing the nulls in `Entitlement.encode(to:)` unconditionally changed
every consumer of that encoding, not just the one that needed it: the
enrichment request body, the paywall template variables, the session
attributes and the public `getDeviceAttributes()` all started carrying
six nulls per bare entitlement.

The encoder now opts in through a `reportsUnknownFieldsAsNull` userInfo
key, threaded from `makeAudienceFilterAttributes` through
`getDeviceAttributes` and `getTemplateDevice` into
`DeviceTemplate.toDictionary`. Everything else encodes exactly as it did
before, which a test pins.

The enrichment endpoint turned out to accept the nulls anyway - it runs
no validation on the device payload and never reads the entitlement
fields - but there is no reason for the other surfaces to carry them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ The scoping is right and the wiring checks out — one coverage gap and one stale file name.

Reviewed changes — the delta from 1647e0e to 9539a22, which turns the previously unconditional null-encoding into an opt-in threaded from the audience-filter attribute factory down to Entitlement.encode(to:).

  • Null-encoding is now per-encoder, not globalCodingUserInfoKey.reportsUnknownFieldsAsNull plus a JSONEncoder.reportingUnknownFieldsAsNull() factory replace the unconditional encodeNilOrValue, and Entitlement.encode(to:) reads the flag off the encoder it was handed.
  • Opt-in threaded through four hopsmakeAudienceFilterAttributesDeviceHelper.getDeviceAttributesgetTemplateDeviceDeviceTemplate.toDictionary(encoder:), with false defaults at every level so existing call sites are untouched.
  • The downstream-consumer question from both prior reviews is closed — the enrichment request body (DeviceHelper.swift:936), makeJsonVariables (DependencyContainer.swift:359), session attributes (DependencyContainer.swift:275) and the public getDeviceAttributes() (Superwall.swift:312) all take the default and keep their pre-PR shape. unknownFieldsStayOmittedForEveryOtherConsumer pins that. PresentationRequest.expression_params does now carry nulls, but it is JSONSerialization-encoded into a String before entering the params dict, so no NSNull reaches host-app delegates or the RN/Expo/Flutter bridges.
  • CHANGELOG entry updated to state the scoping explicitly.
  • Two new testsunknownFieldsStayOmittedForEveryOtherConsumer and theNullFlagReachesNestedEntitlements; both fail on revert of the respective production change.

I verified the two things the new design rests on. makeAudienceFilterAttributes really is the only route from an Entitlement to a CEL evaluation: CELEvaluator is the sole ExpressionEvaluating conformer and the sole evaluateWithContext call site, DependencyContainer is the sole AudienceFilterAttributesFactory, and both audience entry points (AudienceLogic.swift:117, ConfigLogic.swift:231) share the one evaluator from ConfigManager.swift:79. And userInfo propagation into nested encoders is contractual, not incidental — swift-foundation mints children as __JSONEncoder(options: self.options, ownerEncoder: self) and swift-corelibs-foundation as JSONEncoderImpl(options: self.options, …); neither resets userInfo between levels, and the SDK's iOS 13 floor is far past that. The new encode(_:forKey:nilAsNull:) overload can't disturb any existing encode(_:forKey:) call site either — different arity, required third label.

ℹ️ Nitpicks

  • Sources/SuperwallKit/Misc/Extensions/KeyedEncodingContainer+EncodeNilOrValue.swift no longer contains an encodeNilOrValue, and three of its four declarations aren't KeyedEncodingContainer extensions at all. Something like Encoder+ReportsUnknownFieldsAsNull.swift would match what's in it — worth folding in while the pbxproj is already being regenerated.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread Sources/SuperwallKit/Dependencies/DependencyContainer.swift
yusuftor and others added 2 commits September 7, 2026 16:45
Every other case builds the encoder by hand, so deleting
`reportingUnknownFieldsAsNull: true` from `makeAudienceFilterAttributes`
left the whole suite green while restoring the incident. The new test
runs the real production call and checks both directions: the filter
attributes carry a null `willRenew`, and `getTemplateDevice` still omits
the key. Verified it fails when that argument is removed.

Also renames the helper file, which no longer holds an `encodeNilOrValue`
and is mostly not a `KeyedEncodingContainer` extension.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the delta from 9539a22 to f60d255, which closes both outstanding items from the prior review. No production behaviour changed in this delta.

  • The one enabling argument is now pinnedonlyTheAudienceFilterPathReportsUnknownFieldsAsNull sets Superwall.shared.customerInfo to a single bare entitlement and drives the real makeAudienceFilterAttributes(forPlacement:withComputedProperties:), asserting device.customerInfo.entitlements[…]["willRenew"] is present and NSNull, then that getTemplateDevice() still omits the key. Deleting reportingUnknownFieldsAsNull: true from DependencyContainer.swift:474 now fails a test instead of silently restoring the incident, and the #require on the entitlement means a shape change fails rather than being absorbed.
  • Extension file renamed to Encoder+ReportsUnknownFieldsAsNull.swift, contents unchanged. A repo-wide grep finds no remaining EncodeNilOrValue / encodeNilOrValue reference in Swift, project.yml, the pbxproj, or docs.
  • pbxproj regenerated — all four registrations (build file, file reference, group child, Sources phase) are present for the renamed file, plus the test file in the test target.

I checked the two things the new test rests on. DeviceHelper.getTemplateDevice reads Superwall.shared.customerInfo directly (:1017), so the global write really is the only way to feed the real chain; its didSet only consults testModeManager (nil under test) and Superwall.shared resolves to a bare Superwall() with no listeners when SUPERWALL_UNIT_TESTS is passed, so the write has no side effects. The global mutation is also safe from cross-suite interleaving: scripts/test.sh runs the SuperwallKit scheme, whose SuperwallKitTests testable is parallelizable = "NO", and the suite is .serialized on top of that. Finally, the enrichment merge is merge(enrichmentDict) { current, _ in current } (DeviceHelper.swift:1047), so cached enrichment can neither overwrite nor strip the nulls the assertion depends on.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@yusuftor
yusuftor merged commit 1b24185 into develop Sep 7, 2026
@yusuftor
yusuftor deleted the fix/entitlement-unknown-fields branch September 7, 2026 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant