feat: [SDK-5024] add composite login(externalId, profile) as one Create User POST - #2743
abdulraqeeb33 wants to merge 10 commits into
Conversation
…te User POST Apply email, SMS, tags, and aliases in a single upsert so identity-only login stays unchanged, and surface HTTP 4xx on the waiting OneSignalResult instead of pausing the op repo. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
📊 Diff Coverage ReportDiff Coverage Report (Changed Lines Only)Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff). Changed Files Coverage
Overall (aggregate gate)295/336 touched executable lines covered (87.8% — requires ≥ 80%) Per-file detail (informational; gate is aggregate above):
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review (Claude Opus 5, GPT 5.6 Sol, Cursor Grok 4.6) on the composite login(externalId, profile) change. Substantive diff is +1,331/−64 (1,333 after excluding generated core.api).
Act on
- Composite 400/409 drops the whole grouped batch (3/3).
FAIL_NORETRY→dropAndWake(ops)removes login and grouped push create/transfer ops. AfterswitchUserhas already replaced local identity, a bad email/SMS can strand the user on a local OneSignal ID with no push create. Fail the waiter without dropping followers, or revert the switch. - Same-user composite login can return success before HTTP (3/3). Dedupe by
onesignalIdwakes a second waiter withOperationWaitResult(true)when the queued op already has a waiter. Combined with first-winsmergeProfileFromon email/phone, a laterlogin(id, profile)reports success while its address is discarded, and a later 400 never reaches that caller. - Same-user context is captured off the login lock (2/3).
contextForCurrentUserandloginDataFromStoresread live identity/subscription stores. Concurrent login/logout can pair profile A with user B, or return{externalId: A, onesignalId: B}.
Consider
- Full profile plus raw HTTP bodies are logged (3/3).
- Reserved-only aliases still count as profile fields, so same-user login is no longer a no-op and 4xx uses
FAIL_NORETRY(2/3). - Any profile field skips the anonymous→identified
SetAliasmerge (Opus). persistProfileSubscriptionhardcodesSUBSCRIBED/optedIn=true; match-by-type can bind the wrong email/SMS (Opus/Grok).backendCodeis the HTTP status, not a catalog code; 404/410 still pauses the repo (Opus).
Noted / dismissed
OneSignalUserProfile as a public data class (copy/componentN API surface), constructor binary breaks on ExecutionResponse/LoginUserOperation, hydrateProfile writing the request not the response, and catalog backendCode parsing — last is explicitly out of scope.
Sent by Cursor Automation: PR Reviews
| NetworkUtils.ResponseStatusType.INVALID, | ||
| NetworkUtils.ResponseStatusType.CONFLICT, | ||
| -> | ||
| if (createUserOperation.hasProfileFields()) { | ||
| backendExecutionResponse(ExecutionResult.FAIL_NORETRY, ex) |
There was a problem hiding this comment.
Act on (3/3): Composite 400/409 returns FAIL_NORETRY, and OperationRepo then dropAndWakes the entire grouped batch — not just this login.
LoginUserOperation, CreateSubscriptionOperation, and TransferSubscriptionOperation share createComparisonKey = "$appId.User.$onesignalId". After a new-user switchUser, the push create is in that group. A malformed SMS/email therefore drops the device subscription create while local identity already shows the new externalId and a still-local onesignalId.
A retry takes the same-user path and POSTs profile-only, so push never reattaches. Identity-only 4xx still pause and keep the ops; this path should fail the waiter without dropping grouped followers, or roll the local switch back.
…Repo LoginData lookup lives on LoginHelper, profile emptiness on OneSignalUserProfile, nested properties use JSONUtils.mapToJson, and wait results are an ExecutionResponse extension. Co-authored-by: Cursor <cursoragent@cursor.com>
| ) | ||
|
|
||
| OutlineButton( | ||
| text = "LOGIN WITH PROFILE", |
There was a problem hiding this comment.
this needs to be added to iOS and all the other wrappers
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…etadata Failed composite login dropped the op after switching identity, so a retry no-opped. Multi-waiter dedupe now wakes every waiter with the real result, and LoginData comes from typed wait metadata instead of live stores. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
The repo stores heterogeneous waiters, so T was erased and cast back. A sealed payload keeps enqueue-and-wait reusable without a typed lie on IOperationRepo. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Potential issues:
|
…, Any?> createUser already only sent timezone, language, and tags. Use the same typed properties object as updateUser so tags are not stuffed through Any?. Co-authored-by: Cursor <cursoragent@cursor.com>
| Log.d(TAG, "Logged in user with onesignalId: ${OneSignal.User.onesignalId}") | ||
| } | ||
|
|
||
| suspend fun loginUserWithProfile( |
There was a problem hiding this comment.
maybe should be loginWithProfile? maybe could vote on it
not blocking
4af465a to
580da82
Compare
Composite FAIL_NORETRY drops only the login. Paused and in-flight duplicates attach without a second Create User. Co-authored-by: Cursor <cursoragent@cursor.com>
|
On 3: leaving waiters on the shared result. We want composite login to match the existing login methods (one Create User per onesignalId, attached waiters get that result) and not add a per-caller profile path. |
|
still a potential issue: |
nan-li
left a comment
nan-li
left a comment
There was a problem hiding this comment.
Fadi's post-create point is still open, and I reproduced the 4xx case on a device.
-
Second login hangs (Fadi's point).
-
Profile silently dropped. A
login(id, profile)that arrives while a plainlogin(id)for the same user is in flight reuses that result, so the email and phone are never sent. -
User stuck after a 400. The login is dropped, but the push subscription and anything added afterwards stay queued under a local id that is never resolved, so nothing for this user is sent again. Reproduced by calling login("1", profile).
-
Anonymous user no longer merged. With a profile attached, a new external id from an anonymous user creates a second user and leaves the anonymous one behind with its tags. May need to be called in our docs and PR as behavior change.
-
Repeat logins with the same email and phone. The backend omits giving ID for unchanged subscriptions, so a repeat login logs an error. It is common for a client to always login with the profile they have on hand for the user, which will trigger this error log often. Reproduced by calling
login(user, existing email or sms)
| */ | ||
| class OneSignalUserProfile @JvmOverloads constructor( | ||
| email: String? = null, | ||
| phoneNumber: String? = null, |
There was a problem hiding this comment.
Since our API is currently OneSignal.User.addSms(sms: String), we should be consistent and maybe do sms or smsNumber instead of phoneNumber?
| Logging.error("LoginUserOperationExecutor.createUser response is missing a local subscription model for ${pair.first}") | ||
| } | ||
| } else { | ||
| Logging.error("LoginUserOperationExecutor.createUser response is missing subscription data for ${pair.first}") |
There was a problem hiding this comment.
normally in the past this would fire for a genuinely missing push sub but now with the profile including email and/or sms regularly, this will log an error with __profile_email or __profile_sms if that email/sms already belonged on this user. Similar to SDK-5181
3 participants
Description
One Line Summary
Add
login(externalId, profile)so email, SMS, tags, and aliases go out on a single Create User upsert, and return HTTP 4xx on the waitingOneSignalResult.Details
Motivation
Identity-only
login(externalId)cannot attach profile fields. Composite login needs those fields on one Create User POST (upsert byexternal_id) so a bad SMS/email cannot freeze the op repo, and the suspend caller gets the backend body instead of a generic "did not complete".Stacked on #2710 (
ar/sdk-4988). Parent: SDK-5023. Types/signature: SDK-5024. One POST: SDK-5025.Scope
OneSignalUserProfile(email,phoneNumberin E.164,tags,aliases) plus JavaBuilder. JWT stays a separate argument.externalIdstays the login key.login/loginSuspendare unchanged.externalIdwith no profile fields is still a no-op. SameexternalIdwith profile fields upserts.FAIL_NORETRY) instead of pausing the repo. Identity-only 4xx still pause.enqueueAndAwaitResultcarries HTTP status/body so compositeloginreturnsOneSignalResult.failure(BACKEND_ERROR, message=body, backendCode=HTTP status). Catalog codes are not parsed yet.Not in this PR: persistence (SDK-5026), telemetry (SDK-5027), a login callback, catalog
backendCode.Testing
Unit testing
Executor tests cover identity-only payload unchanged, profile aliases/email/SMS/tags on Create User, reserved alias skip, 400/409
FAIL_NORETRYwith HTTP fields, 401, and hydration. OperationRepo wakesenqueueAndAwaitResultwith status/body. LoginHelper, UserBackendService nested tags, and profile builder tests added.Manual testing
Demo app on a physical device. Composite login with a non-E.164 SMS (
4129089471) returns HTTP 400Invalid token format for device type SMS, logs the body, returnsOneSignalResult.failure, and does not pause the op repo. Retry with+14129089471succeeds.Affected code checklist
Checklist
Overview
Testing
Final pass
Made with Cursor