Skip to content

fix: convert a JSON null to null, not to the text "null" - #459

Merged
ianrumac merged 2 commits into
developfrom
fix/entitlement-unknown-fields
Sep 8, 2026
Merged

fix: convert a JSON null to null, not to the text "null"#459
ianrumac merged 2 commits into
developfrom
fix/entitlement-unknown-fields

Conversation

@yusuftor

@yusuftor yusuftor commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

What

JsonElement.toPassableValue() turned a JSON null into the string "null":

is JsonPrimitive -> when {
    this.isString -> StringValue(this.content)
    this.booleanOrNull != null -> ...
    else -> StringValue(this.content)   // JsonNull lands here
}

JsonNull is a JsonPrimitive whose isString is false and whose content is "null", so it falls past every check into the catch-all. An audience filter asking field == null never matched, while field == "null" did.

Fixed with an is JsonNull -> PassableValue.NullValue branch ahead of the primitive check (order matters — JsonNull is a JsonPrimitive).

The sibling converter convertFromJsonElement in Converters.kt already does exactly this, so the two paths now agree. PassableValue.NullValue already existed, and the map/list branches of the Any? overload already produce it for Kotlin nulls — only the JsonElement overload was missing it.

When this actually bites

Only when a value is already a kotlinx JsonElement on its way into Any.toPassableValue(). I traced where those can come from:

  • Device attributes — no. toNullableTypedMap runs everything through convertFromJsonElement first, so they arrive as basic Kotlin types.
  • SDK-internal event params and user attributes — no. Nothing internal puts a JsonElement in either.
  • Host-app supplied values — yes. A Kotlin app using kotlinx.serialization for structured attributes or placement params.

Measured, for what a host app can actually store:

what the app passes stored? before after
JsonObject with a nested null field yes — top-level value is a real object StringValue("null") NullValue
the value is JsonNull yes — see below StringValue("null") NullValue
the value is a Kotlin null no — key is removed never reaches the converter

The second row is the subtle one. IdentityLogic.mergeAttributes deletes an attribute whose value is null, which is why a real null never gets this far. But JsonNull is an object, so value != null is true — it slips past that guard, gets stored, and reaches the broken branch. The one thing protecting against nulls is the thing that misses this one.

So: a host app storing profile data built from an API response with a null field gets attributes.profile.company == null never matching, while == "null" does.

Heads up: two existing tests pinned the old behaviour

JsonElementToPassableValueTest asserted the bug in two places:

assertTrue(resultMap["null"] is PassableValue.StringValue)
assertEquals("null", (resultMap["null"] as PassableValue.StringValue).value)

Both now expect NullValue. A repo-wide grep found nothing else depending on the string output, and PassableValueTest already expected NullValue for the Kotlin-null path.

Relationship to the iOS entitlement fix — none, despite appearances

This started while looking at Superwall-iOS#517, where a Purchase Controller's entitlement reported willRenew as absent, the evaluator gave the missing key the type default false, and active subscribers matched a "will not renew" audience.

That bug does not exist on Android, and this PR is not its counterpart. Android's customer_info path is already null-correct end to end:

CustomerInfo.toParams()
  → JsonFactory.JSON (explicitNulls defaults to true)  → "willRenew": null
  → convertFromJsonElement()                           → JsonNull → Kotlin null
  → Any.toPassableValue() map branch                   → NullValue

It resolves through convertFromJsonElement, which has always handled JsonNull, and never enters the converter this PR fixes.

For the record, since an earlier draft of this description got it wrong: entitlement filtering does work on Android. device.activeEntitlements (ids) and device.activeEntitlementsObject (identifier + type) are available on every placement, and full entitlement detail — willRenew, state, offerType, dates — is reachable as params.customer_info on paywall-generated placements, via PaywallInfo.eventParams(). The real difference from iOS is that iOS exposes device.customerInfo on every placement, so an audience written as device.customerInfo.entitlements... binds on iOS and silently doesn't on Android. That parity gap is not addressed here.

Testing

  • ./gradlew :superwall:testDebugUnitTest — BUILD SUCCESSFUL, full suite.
  • New test JsonNull conversion to PassableValue, plus the two corrected assertions covering a null inside a JsonObject and inside a JsonArray.

🤖 Generated with Claude Code

yusuftor and others added 2 commits September 7, 2026 15:27
`JsonElement.toPassableValue` matched `JsonNull` against its
`is JsonPrimitive` branch, where every check failed and it fell to the
catch-all that reads `content` - which for a null is the string "null".
An audience filter asking `field == null` never matched, while
`field == "null"` did.

The sibling converter `convertFromJsonElement` already handles this, so
the two now agree.

Two existing assertions in JsonElementToPassableValueTest pinned the old
behaviour and are updated.

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

yusuftor commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

@pullfrog

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No correctness issues found — one coverage suggestion below.

Reviewed changes — the JsonNull branch added to JsonElement.toPassableValue(), the three test assertions it changes, the changelog entry, and the automated coverage badge commit.

  • JsonNull no longer becomes the string "null"SuperscriptEvaluator.kt:250-253 adds is JsonNull -> PassableValue.NullValue ahead of the is JsonPrimitive branch. Branch order is required and correct: JsonNull is a JsonPrimitive whose isString is false and whose content is "null", so it previously fell into the catch-all else -> StringValue(this.content) at line 272.
  • Two tests that pinned the old behaviour are correctedJsonElementToPassableValueTest.kt:105 and :138 now expect NullValue for a null inside a JsonObject and inside a JsonArray. Both assertions genuinely fail without the fix, as does the new test JsonNull conversion to PassableValue at :67-73.
  • Changelog + coverage badgeCHANGELOG.md entry under ## Unreleased; the jacoco.svg change is this repo's automated Update coverage badge [skip ci] commit, matching the three prior badge commits on develop.

I checked the two things the fix rests on and both hold:

  • The branch is reachable in production, not dead code. setUserAttributes does no type conversion (cleanAttributes in PublicIdentity.kt only strips $-prefixed keys) and IdentityLogic.mergeAttributes:40 gates on Kotlin null, which the JsonNull singleton passes. Placement params reach it too: TrackingLogic.clean:144 returns is Map<*, *> -> value without recursing, so a JsonNull nested in a map value survives to the is JsonElement -> branch at SuperscriptEvaluator.kt:221. The paths that don't need this branch — Enrichment.kt, DeviceTemplate.toDictionary, the Room Converters — all convert to Kotlin null first and are handled by the existing ?: PassableValue.NullValue fallbacks.
  • The NullValue wire shape is already load-bearing. PassableValue.NullValue encodes to {"type":"Null"} under the ClassDiscriminatorMode.POLYMORPHIC / classDiscriminator = "type" config in DependencyContainer.kt:229-232, and the map/list branches at SuperscriptEvaluator.kt:193/204/216 already emit it for Kotlin nulls today. This change routes more values onto an existing contract rather than introducing a new one.

ℹ️ Nothing tests the behaviour the changelog promises

Every added assertion stops at the Kotlin converter. The user-visible claim — that an audience filter checking field == null now matches — depends on the Rust supercel binary decoding {"type":"Null"} into a CEL null and comparing it equal to the null literal, which no test in the repo exercises. The risk is low because the existing Kotlin-null paths already emit NullValue, but a silent regression here degrades audience targeting for every Android user, and SuperscriptExpressionEvaluatorInstrumentedTest already has the harness to pin it.

Technical details
# Add an evaluator-level test for `== null` matching

## Affected sites
- `superwall/src/androidTest/java/com/superwall/sdk/paywall/presentation/rule_logic/expression_evaluator/SuperscriptExpressionEvaluatorInstrumentedTest.kt` — has `evaluatorFor(...)` plus a `test_happy_path_evaluator` case using `expressionCEL = "user.id == \"123\""`; no case covers a null-valued attribute.
- `superwall/src/test/java/com/superwall/sdk/paywall/presentation/rule_logic/JsonElementToPassableValueTest.kt:67-73` — covers the converter output only, so it cannot catch a Kotlin↔Rust wire mismatch.

## Required outcome
- A test that fails if `PassableValue.NullValue` stops round-tripping through the native evaluator as a CEL null — i.e. it drives a real `evaluateExpression` with an attribute whose JSON value is null and asserts the rule matches.

## Suggested approach (optional)
- Add a case alongside `test_happy_path_evaluator` with `expressionCEL = "params.foo == null"`, feeding `EventData(parameters = mapOf("obj" to mapOf("foo" to JsonNull)), ...)` (nested in a map, since `TrackingLogic.clean` drops a top-level `JsonElement` null) or a `RuleAttributeFactoryBuilder` variant that supplies a `JsonNull` attribute directly.
- Assert `TriggerRuleOutcome.match(rule = rule)`, and confirm the test fails when `SuperscriptEvaluator.kt:253` is reverted.

## Open questions for the human
- Is the `supercel` version pinned in `gradle/libs.versions.toml` (`supercel_version = "1.0.13"`) built from a `superscript` revision that handles the `Null` unit variant? I could not tie the published AAR back to a tagged Rust source revision, so an instrumented test is the only way to confirm it against the binary actually shipping.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

@ianrumac
ianrumac merged commit 29ace48 into develop Sep 8, 2026
1 check passed
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.

2 participants