fix: [SDK-5099] respect REST API-disabled push subscriptions - #2728
fix: [SDK-5099] respect REST API-disabled push subscriptions#2728nan-li wants to merge 1 commit into
Conversation
📊 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)35/51 touched executable lines covered (68.6% — requires ≥ 80%) Per-file detail (informational; gate is aggregate above):
❌ Coverage Check FailedAggregate coverage on touched lines is 68.6% (minimum 80%). |
A push subscription disabled through the REST API (notification_types -31) was re-enabled by the SDK: RefreshUser discarded the server's disable state for push, the session-start self-heal re-asserted local truth over it, and every subscription payload recomputed enabled from device state. Mirror the server's disable code on the push model when RefreshUser reports it, report it back in subscription payloads instead of the device-derived values, skip the stuck-subscription self-heal for it, and carry it across the login/logout user switch. The mirror clears when the server reports any other state and on an explicit optIn(). The 404 recovery paths (user rebuild and update-404 re-create) treat the dead record's disable as gone and recreate from device truth. Also remove the mislabeled DISABLED_FROM_REST_API_DEFAULT_REASON(-30) enum case; no OneSignal API has ever written -30 as a REST disable. Enum-name persistence now parses leniently in the shared model accessor, so models and queued operations persisted under an unknown enum name read as SUBSCRIBED instead of throwing on upgrade.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review
Models: Claude Opus 5, GPT 5.6 Sol, Cursor Grok 4.6.
The -31 mirror, self-heal skip, payload reporting, optIn() clear, and 404 device-truth recreate are internally consistent. Two holes can still re-enable a REST-disabled push subscription — the bug this PR is meant to close.
Act on
- Session-start / queued update wins over hydrate (2/3).
getUpdateOperationfreezesenabled/statusat enqueue time. On first launch of this build,SubscriptionManager.refreshPushSubscriptionStatewrites a newsdk(NORMAL) whilerestApiDisabledReasonis still0, enqueueingUpdateSubscriptionOperation(enabled=true, SUBSCRIBED). RefreshUser then records-31, but that already-queued PATCH still re-enables. Same shape for any token/permission write while the GET is in flight. Snapshot at execute time, or drop/rewrite in-flight push updates once-31is recorded. - 4.x→5.x legacy sync never sets
restApiDisabledReason(3/3).createPushSubscriptionFromLegacySyncnow parses-31into devicestatusand leaves the new field at0.addOrUpdatePushSubscriptionTokenthen overwritesstatuswithSUBSCRIBED, and the next payload is enabled. SeedrestApiDisabledReasonwhenisRestApiDisable(notificationTypes). - 404 recovery fallback recreates the dead disable (2/3). If the cached model is missing, recovery uses
lastOperation.enabled/status, which can be(false, DISABLED_FROM_REST_API). That contradicts “the dead record’s disable is gone.” Do not reuse-31from the failed op.
Consider
- Unknown persisted enum names (including the removed
-30case) fall back toSUBSCRIBED(3/3). Fail-open can re-enable on downgrade;ERRORis safer for modelstatus. notificationTypes == nullleaves a recorded-31in place but does not skip self-heal (3/3). Treat omit as unknown: no clear and no self-heal.optIn()then a still-in-flight RefreshUser GET can write-31back after the clear (Opus). PublicoptedInwill not show it.- Opt-out (or permission loss) during
-31is never sent; when the server later clears-31, hydrate isHYDRATEso nothing re-asserts local opted-out (Opus).
Noted / dismissed
optIn()tests!= 0instead ofisRestApiDisable— same today; they diverge if a second code is added.- Detekt baseline still lists
DISABLED_FROM_REST_API_DEFAULT_REASON$30. One new MagicNumber is unlikely to fail CI (maxIssues: 10); still worth regenerating the baseline. - Leftover pre-upgrade self-heal ops: possible if the last PATCH never ran, not “almost certainly” still queued.
- No
RebuildUserServicetest for the 404 clear; no test that publicoptedInstays true while-31is recorded.
Sent by Cursor Automation: PR Reviews
| ) { | ||
| val cachedPushSubscriptionModel = _subscriptionsModelStore.get(pushSubscriptionId) ?: return | ||
| val serverTypes = serverSubscription.notificationTypes ?: return | ||
| // The recorded reason mirrors the server's field: -31 records, any other reported value clears. | ||
| val target = if (SubscriptionStatus.isRestApiDisable(serverTypes)) serverTypes else 0 | ||
| if (cachedPushSubscriptionModel.restApiDisabledReason != target) { | ||
| cachedPushSubscriptionModel.setIntProperty( | ||
| SubscriptionModel::restApiDisabledReason.name, | ||
| target, | ||
| ModelChangeTags.HYDRATE, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| companion object { |
There was a problem hiding this comment.
Act on (2/3): queued updates still re-enable after this hydrate.
getUpdateOperation snapshots enabled/status at enqueue time. On first launch of this build, SubscriptionManager.refreshPushSubscriptionState writes a new sdk while restApiDisabledReason is still 0, so a NORMAL UpdateSubscriptionOperation(enabled=true, SUBSCRIBED) can already be queued before this GET returns. Hydrate then records -31, but the in-flight PATCH still re-enables the subscription this PR is meant to leave suppressed.
Same race for any token/permission write while RefreshUser is outstanding. Snapshot at execute time, or drop/rewrite in-flight push updates once -31 is recorded.
Also: notificationTypes ?: return leaves a recorded -31 in place, but isRestApiDisable(null) does not skip self-heal. Treat omit as unknown (no clear and no self-heal).
| optedIn = currentPushSubscription?.optedIn ?: true | ||
| address = currentPushSubscription?.address ?: "" | ||
| status = currentPushSubscription?.status ?: SubscriptionStatus.NO_PERMISSION | ||
| restApiDisabledReason = currentPushSubscription?.restApiDisabledReason ?: 0 |
There was a problem hiding this comment.
Act on (3/3): this copy is good; the 4.x→5.x path in this class is not.
createPushSubscriptionFromLegacySync still never sets restApiDisabledReason. A legacy notification_types == -31 now lands in device status (because fromInt(-31) resolves) with optedIn = true and reason 0. SubscriptionManager.addOrUpdatePushSubscriptionToken then overwrites status with SUBSCRIBED, and the next payload is enabled — the silent re-enable this PR is fixing, on the one entry path that does not populate the new field.
If isRestApiDisable(notificationTypes), set restApiDisabledReason and keep device status as the real device status.
| val (recoveryEnabled, recoveryStatus) = | ||
| recoveryModel?.let { SubscriptionModelStoreListener.getSubscriptionEnabledAndStatus(it) } | ||
| ?: Pair(lastOperation.enabled, lastOperation.status) |
There was a problem hiding this comment.
Act on (2/3): model-missing fallback recreates the dead disable.
Clearing -31 and recreating from device truth only runs when recoveryModel exists. If get(staleSubscriptionId) is null, this falls back to lastOperation.enabled/status, which can be (false, DISABLED_FROM_REST_API) and POSTs the deleted record’s disable back.
When the model is missing, still discard DISABLED_FROM_REST_API rather than reusing the failed op’s pair.
|
|
||
| return getEnumProperty(::status.name) | ||
| // A persisted name this build's enum lacks reads as SUBSCRIBED instead of throwing. | ||
| return getOptEnumProperty<SubscriptionStatus>(::status.name) ?: SubscriptionStatus.SUBSCRIBED |
There was a problem hiding this comment.
Consider (3/3): fail-open fallback.
An unknown persisted name (including the removed DISABLED_FROM_REST_API_DEFAULT_REASON) becomes SUBSCRIBED. With optedIn == true and a token, getSubscriptionEnabledAndStatus reports enabled. That is the same silent re-enable class this PR is fixing, on the upgrade/downgrade path.
ERROR is a safer model-status fallback (isRetryableTokenError == false). The added test only asserts the fallback value, not the resulting payload.
Holistic multi-model review (not just the diff)Models: Claude Opus 5, GPT 5.6 Sol, Claude Sonnet 5 — findings independently re-checked against the subscription lifecycle, operation queue, and iOS #1731. Request changes. The Act on1. Upgrade session PATCHes
Same shape for token rotation while RefreshUser is in flight, and for persisted pre-upgrade ops. iOS builds PATCH / Create User from a live snapshot ( 2. 4.x→5.x legacy sync never sets
If 3.
That Create groups with iOS clears when the subscription ID resets, which is the right trigger. This path has no unit test ( Consider
Product calls (PR asked reviewers to confirm; iOS matches)
What is solidA separate Tests that would have caught the Act-on itemsSession-start upgrade (sdk write + RefreshUser Traced beyond the 14-file diff: |
| externalId: String?, | ||
| pushSubscription: SubscriptionModel, | ||
| ): CreateSubscriptionOperation { | ||
| _subscriptionsModelStore.get(pushSubscription.id)?.setIntProperty( |
There was a problem hiding this comment.
Act on (2/3): this clears a live disable, not a dead one.
getRebuildOperationsIfCurrentUser runs on 404 from GET /users, PATCH /users, alias ops, and create-subscription — none of those independently prove this push row is gone. This HYDRATE-clears restApiDisabledReason on the live store, then emits CreateSubscriptionOperation with the existing id and device-derived enabled=true.
That Create groups with LoginUserOperation (createComparisonKey = "$appId.User.$onesignalId") and becomes a POST /users subscription with the live id. If it later runs alone, updateExistingSubscriptionFromCreate PATCHes the live row.
iOS clears when the subscription ID resets. Only clear here when IDManager.isLocalId(pushSubscription.id), or drop the store mutation and let the trailing RefreshUser re-establish truth. This file also has 0% diff coverage (16/16 uncovered), which is why the aggregate gate is 68.6%.
nan-li
changed the title
1 participant
Description
One Line Summary
Stop re-enabling push subscriptions that were disabled through the REST API (
notification_types-31).Details
Motivation
Customers who suppress users by disabling subscriptions via the REST API see them come back subscribed. RefreshUser discarded the server's disable state for push, the session-start self-heal (SDK-4474) re-asserted local truth over it on every app open, and every subscription payload recomputed
enabledfrom device state. Tracked internally as SDK-5099.Scope
Push subscriptions only. The push model gains a server-owned
restApiDisabledReasonfield that mirrors the server'snotification_types: RefreshUser records -31 and clears on any other reported value,getSubscriptionEnabledAndStatusreports(false, -31)while recorded so create/update/login payloads preserve it, the self-heal skips it, and it is carried across the login/logout user switch.optIn()clears it. The 404 recovery paths (user rebuild and update-404 re-create) treat the dead record's disable as gone and recreate from device truth.Also removes the mislabeled
DISABLED_FROM_REST_API_DEFAULT_REASON(-30)enum case; no OneSignal API writes -30 as a REST disable (the docs list -30 as an APNs error, and the actual dashboard/player disable code is -22). Because models and operations persist enum properties by name,Model.getOptEnumPropertynow parses leniently so anything persisted under an unknown enum name reads asSUBSCRIBEDinstead of throwing on upgrade. AddingDISABLED_FROM_REST_API(-31)also fixes -31 email/SMS rows previously misparsing toSUBSCRIBED.For reviewers
A server-side disable deliberately does not flip the public
optedIn, andoptIn()deliberately overrides the suppression. Both are product calls worth confirming, and they should match iOS (companion PR in OneSignal-iOS-SDK).Testing
Unit testing
New coverage across four suites: RefreshUser recording/mirror-clearing and self-heal skip, payload reporting via
getSubscriptionEnabledAndStatus,optIn()clearing, user-switch carry-over, update-404 recovery from device truth, and lenient parsing of unknown persisted enum names for both models and operations (throughinitializeFromJson).Manual testing
Not device-tested; the reported scenarios are reproduced by the executor and user-switch tests above.
Affected code checklist
Checklist
Overview
Testing
Final pass