feat: add android install attribution matching#389
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| val matched: Boolean, | ||
| val confidence: AttributionMatchInfo.Confidence? = null, | ||
| val matchScore: Double? = null, | ||
| val clickId: Int? = null, |
There was a problem hiding this comment.
clickId typed as Int may overflow or throw during deserialization
The request field installReferrerClickId is correctly typed as Long?, but the corresponding response field is typed as Int?. Play Store install-referrer click IDs are large 64-bit integers. If the server echoes back or assigns a click ID that exceeds Int.MAX_VALUE (~2.1 billion), kotlinx.serialization will throw a SerializationException when deserializing the response JSON — even with coerceInputValues = true, which only coerces enum/null values, not numeric widths. This would cause the match to fail and the attribution event to be tracked with reason request_failed.
| val clickId: Int? = null, | |
| val clickId: Long? = null, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: superwall/src/main/java/com/superwall/sdk/network/MmpService.kt
Line: 38
Comment:
**`clickId` typed as `Int` may overflow or throw during deserialization**
The request field `installReferrerClickId` is correctly typed as `Long?`, but the corresponding response field is typed as `Int?`. Play Store install-referrer click IDs are large 64-bit integers. If the server echoes back or assigns a click ID that exceeds `Int.MAX_VALUE` (~2.1 billion), kotlinx.serialization will throw a `SerializationException` when deserializing the response JSON — even with `coerceInputValues = true`, which only coerces enum/null values, not numeric widths. This would cause the match to fail and the attribution event to be tracked with reason `request_failed`.
```suggestion
val clickId: Long? = null,
```
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| private fun readJsonString( | ||
| value: Map<String, JsonElement>?, |
There was a problem hiding this comment.
jsonPrimitive cast can throw IllegalArgumentException
The jsonPrimitive extension property on JsonElement is non-nullable and throws IllegalArgumentException if the element is a JsonObject or JsonArray rather than a JsonPrimitive. The null-safe ?. operator only guards against a null JsonElement, not against a wrong type. If the backend ever returns a non-string value for acquisition_source (in acquisitionAttributes) or reason (in breakdown), this will throw inside the coroutine launched by recordMMPInstallAttributionRequest, which is outside the withErrorTracking scope in Superwall.kt, risking an unhandled coroutine exception.
Use a safe cast instead:
| } | |
| private fun readJsonString( | |
| value: Map<String, JsonElement>?, | |
| ): String? = (value?.get(key) as? JsonPrimitive)?.contentOrNull |
Prompt To Fix With AI
This is a comment left during a code review.
Path: superwall/src/main/java/com/superwall/sdk/network/Network.kt
Line: 107-110
Comment:
**Unsafe `jsonPrimitive` cast can throw `IllegalArgumentException`**
The `jsonPrimitive` extension property on `JsonElement` is non-nullable and throws `IllegalArgumentException` if the element is a `JsonObject` or `JsonArray` rather than a `JsonPrimitive`. The null-safe `?.` operator only guards against a null `JsonElement`, not against a wrong type. If the backend ever returns a non-string value for `acquisition_source` (in `acquisitionAttributes`) or `reason` (in `breakdown`), this will throw inside the coroutine launched by `recordMMPInstallAttributionRequest`, which is outside the `withErrorTracking` scope in `Superwall.kt`, risking an unhandled coroutine exception.
Use a safe cast instead:
```suggestion
): String? = (value?.get(key) as? JsonPrimitive)?.contentOrNull
```
How can I resolve this? If you propose a fix, please make it concise.| return uri.queryParameterNames.associateWith { key -> | ||
| uri.getQueryParameters(key).map { URLDecoder.decode(it, "UTF-8") } | ||
| } |
There was a problem hiding this comment.
android.net.Uri.getQueryParameters(key) already decodes percent-encoded characters (e.g., %20 → space). Applying URLDecoder.decode again on the already-decoded values causes double-decoding: a value that originally contained an encoded %25 (literal percent sign) would first be decoded to %, and then URLDecoder would try to interpret % as the start of a new escape sequence, potentially throwing a java.lang.IllegalArgumentException or silently garbling the value.
For the numeric sw_mmp_click_id this is harmless today, but it is a latent bug for any future string-valued parameters, and it already affects the code parameter used by checkForReferral.
| return uri.queryParameterNames.associateWith { key -> | |
| uri.getQueryParameters(key).map { URLDecoder.decode(it, "UTF-8") } | |
| } | |
| return uri.queryParameterNames.associateWith { key -> | |
| uri.getQueryParameters(key) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: superwall/src/main/java/com/superwall/sdk/web/DeepLinkReferrer.kt
Line: 166-168
Comment:
**Double URL-decoding of query parameter values**
`android.net.Uri.getQueryParameters(key)` already decodes percent-encoded characters (e.g., `%20` → space). Applying `URLDecoder.decode` again on the already-decoded values causes double-decoding: a value that originally contained an encoded `%25` (literal percent sign) would first be decoded to `%`, and then `URLDecoder` would try to interpret `%` as the start of a new escape sequence, potentially throwing a `java.lang.IllegalArgumentException` or silently garbling the value.
For the numeric `sw_mmp_click_id` this is harmless today, but it is a latent bug for any future string-valued parameters, and it already affects the `code` parameter used by `checkForReferral`.
```suggestion
return uri.queryParameterNames.associateWith { key ->
uri.getQueryParameters(key)
}
```
How can I resolve this? If you propose a fix, please make it concise.| private suspend fun getInstallReferrerParams(timeout: kotlin.time.Duration): Map<String, List<String>> { | ||
| val rawReferrer = | ||
| withTimeoutOrNull(timeout) { | ||
| while (referrerClient?.isReady != true) { | ||
| // no-op | ||
| } | ||
| referrerClient?.installReferrer?.installReferrer?.toString() | ||
| } | ||
|
|
||
| referrerClient?.endConnection() | ||
| referrerClient = null | ||
|
|
||
| return rawReferrer?.getUrlParams() ?: emptyMap() | ||
| } |
There was a problem hiding this comment.
referrerClient.endConnection() may not be called on timeout
The custom property getter for referrerClient returns null when the underlying field is not yet isReady. If getInstallReferrerParams times out before the Play Store service completes its handshake, referrerClient?.endConnection() evaluates as a no-op (the getter returns null), and only the backing field is nulled out. The live InstallReferrerClient binding is abandoned without being explicitly released.
To ensure the connection is always closed, bypass the custom getter by calling endConnection on the backing field directly, or promote the close logic to a dedicated closeClient() helper that accesses field (as is done inside tryConnecting's catch block).
Prompt To Fix With AI
This is a comment left during a code review.
Path: superwall/src/main/java/com/superwall/sdk/web/DeepLinkReferrer.kt
Line: 144-157
Comment:
**`referrerClient.endConnection()` may not be called on timeout**
The custom property getter for `referrerClient` returns `null` when the underlying field is not yet `isReady`. If `getInstallReferrerParams` times out before the Play Store service completes its handshake, `referrerClient?.endConnection()` evaluates as a no-op (the getter returns null), and only the backing field is nulled out. The live `InstallReferrerClient` binding is abandoned without being explicitly released.
To ensure the connection is always closed, bypass the custom getter by calling `endConnection` on the backing field directly, or promote the close logic to a dedicated `closeClient()` helper that accesses `field` (as is done inside `tryConnecting`'s `catch` block).
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Notes
Verification
Greptile Summary
This PR adds Android install attribution matching via a new MMP (Mobile Measurement Partner) fingerprinting flow. On first configure within a 7-day install window, the SDK collects device fingerprint fields (screen dimensions, locale, timezone, etc.) and an optional Play Store install-referrer click ID (
sw_mmp_click_id), posts them to a new/api/matchendpoint, and merges any returnedacquisitionAttributesinto user attributes. A newattribution_matchSuperwall event is fired on both success and failure. The feature is guarded by two new persistent flags (DidCompleteMMPInstallAttributionRequest,IsEligibleForMMPInstallAttributionMatch) so the network call is made at most once per install.\n\nKey findings:\n\n-clickId: Int?inMmpMatchResponseshould beLong?— Play Store referrer click IDs are 64-bit integers; a value exceedingInt.MAX_VALUEwill throw aSerializationExceptionat deserialization time even thoughcoerceInputValues = truedoes not cover numeric widths.\n- Unsafe.jsonPrimitivecast inreadJsonString— ThejsonPrimitiveextension property throwsIllegalArgumentExceptionfor non-primitive JSON values; the exception escapes the outerwithErrorTrackingscope becauserecordMMPInstallAttributionRequestlaunches a detached coroutine, risking an unhandled coroutine exception.\n- Double URL-decoding ingetUrlParams—Uri.getQueryParameters()already percent-decodes values; the subsequentURLDecoder.decode()call causes double-decoding, which can garble values containing a literal%and silently corrupts the existingcodereferral parameter.\n-APPLE_SEARCH_ADSprovider enum is dead code on Android — the variant will never be produced by the SDK and may confuse consumers.\n-referrerClient.endConnection()may be skipped on timeout — the custom property getter returnsnullwhen the client is not yetisReady, so a timed-outgetInstallReferrerParamscall does not explicitly release the underlyingInstallReferrerClientbinding.Confidence Score: 4/5
Safe to merge after fixing the
clickIdtype and the unsafejsonPrimitivecast; remaining findings are P2 clean-ups.Two P1 issues remain: the
Int?type forclickIdinMmpMatchResponsethat can cause a deserialization exception on real-world click IDs, and the unsafe.jsonPrimitivecast inreadJsonStringthat can throw an unhandled exception in a detached coroutine. Both are small, targeted fixes. All other findings are P2 style/correctness suggestions that do not block the primary attribution path.MmpService.kt(clickId type) andNetwork.kt(readJsonString cast) need fixes before merge.Important Files Changed
clickIdin response is typed asInt?instead ofLong?, risking serialization failure for large click IDs.matchMMPInstalland acquisition-attribute merging;readJsonStringuses an unsafe.jsonPrimitivecast that throwsIllegalArgumentExceptionfor non-primitive JSON values.getInstallReferrerParams; addscheckForMmpClickId; double URL-decoding and potentialendConnectionleak on timeout are pre-existing and inherited issues.fetchConfiguration()deliberately moved after identity configure and MMP check; logic is sound.shouldAttemptInitialMMPInstallAttributionMatchandrecordMMPInstallAttributionRequestwith a 7-day eligibility window; gating logic is correct.APPLE_SEARCH_ADSprovider enum value is dead code on Android.timezoneOffsetSeconds,screenWidth,screenHeight,devicePixelRatio, andappInstalledAtMillisas typed properties.MmpServiceinto theNetworkconstructor usingapi.subscription.host; otherwise correct.matchMMPInstallto theSuperwallAPIinterface with a defaultnullparameter; straightforward addition.Sequence Diagram
sequenceDiagram participant SW as Superwall.configure() participant LS as LocalStorage participant DLR as DeepLinkReferrer participant PS as Play Store (InstallReferrer API) participant NET as Network participant MMP as MmpService (/api/match) participant IM as IdentityManager SW->>LS: read(DidTrackAppInstall) → hadTracked SW->>LS: recordAppInstall() SW->>IM: configure() [await] SW->>LS: shouldAttemptInitialMMPInstallAttributionMatch(hadTracked, appInstalledAtMillis) LS-->>SW: true (first install, within 7-day window) SW->>DLR: new DeepLinkReferrer(context, ioScope) DLR->>PS: startConnection() SW->>DLR: checkForMmpClickId() [5s timeout] PS-->>DLR: referrer string (sw_mmp_click_id=...) DLR-->>SW: Result<Long> (click ID or null) SW->>LS: recordMMPInstallAttributionRequest { matchMMPInstall(clickId) } Note over LS: launches async in ioScope SW->>NET: fetchConfiguration() [concurrent] LS->>NET: matchMMPInstall(clickId) [async] NET->>MMP: POST /api/match (device fingerprint + clickId) MMP-->>NET: MmpMatchResponse { matched, acquisitionAttributes, ... } NET->>SW: setUserAttributes(acquisitionAttributes) NET->>SW: track(AttributionMatch event) LS->>LS: write(DidCompleteMMPInstallAttributionRequest, true)Comments Outside Diff (1)
superwall/src/main/java/com/superwall/sdk/analytics/superwall/AttributionMatchInfo.kt, line 121-123 (link)APPLE_SEARCH_ADSis dead code in the Android SDKApple Search Ads is an iOS-only advertising platform and has no equivalent on Android. Shipping this variant in the Android SDK is misleading — consumer code could construct an
AttributionMatchInfowithProvider.APPLE_SEARCH_ADS, but it will never be produced by the SDK itself. Consider removing the variant or annotating it as reserved for future cross-platform use to avoid confusion.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "style: format android attribution change..." | Re-trigger Greptile