feat: [SDK-4988] add OneSignalResult, OneSignalError, and the per-method payloads

[abdulraqeeb33]

Adds the public result model as a pure addition, wired to nothing. Nothing in the SDK returns these types yet — that starts in SDK-4990 — so the type design can be reviewed on its own before any API changes shape around it.

Part of SDK-4783. Ticket: SDK-4988.

Base

Targets ar/sdk-4986, which introduces the .api dump this PR extends. GitHub will retarget to feature/async-first-public-api once 4986 merges. The whole migration lands on that feature branch and merges to main once.

What's here

• OneSignalResult<T> — the envelope: isSuccess, the payload, the error, plus toMap/fromMap projections onto the cross-SDK wire schema.
• OneSignalError — a list of Detail plus the originating Throwable.
• ErrorCode / ErrorSource — the shared code catalog.
• InitData, LoginData, LogoutData, UpdateUserJwtData — the per-method payloads.

enum class ErrorCode(val source: ErrorSource) {
    NOT_INITIALIZED(CLIENT), STORAGE_LOCKED(CLIENT), INVALID_ARGUMENT(CLIENT),
    BACKEND_ERROR(BACKEND), UNKNOWN(CLIENT),
}

class OneSignalError internal constructor(error: List<Detail>, val cause: Throwable? = null) {
    class Detail internal constructor(
        val code: ErrorCode,
        val backendCode: Int? = null,
        val message: String? = null,
    )
    val error: List<Detail> = error.toList()
    val first: Detail get() = error.first()
}

Decisions worth your attention

An enum, not a sealed hierarchy. A sealed ErrorCode.Client.StorageLocked reads well in Kotlin and badly everywhere else — from Java it's ErrorCode.Client.StorageLocked.INSTANCE and can't be switched on, only chained through instanceof. An enum compiles to a real java.lang.Enum, so Java gets a native switch and the wrapper bridges get a name() marshal. The exhaustiveness argument for sealed doesn't apply: adding an enum constant breaks a Kotlin when in exactly the same way.

Backend codes stay a raw Int. The backend adds catalog codes on its own schedule, and an SDK release must not be what unblocks recognizing one. BACKEND_ERROR plus Detail.backendCode keeps that half open-ended.

Detail is nested rather than a top-level Error. A top-level com.onesignal.Error would shadow kotlin.Error, which is auto-imported into every Kotlin file, and java.lang.Error in any Java file that imports it.

No retryable or httpStatus. Both would be guesses the SDK can't currently back — nothing below the entry points reports a status code yet (SDK-4989), and retryability would be hardcoded per call site rather than derived from anything. A field customers branch on, populated by a guess, is worse than no field.

A list, not one reason. One request can fail for several reasons at once. Everything the SDK raises locally has exactly one, which first reads without the indexing ceremony.

cause is off the wire. A stack trace can't cross a wrapper bridge and the schema has to stay identical across SDKs, so cause exists only for native Kotlin and Java callers.

Wire shape

{ "success": false, "data": null,
  "error": [ { "code": "STORAGE_LOCKED", "source": "CLIENT", "backendCode": null, "message": "..." } ] }

fromMap never throws on unexpected input:

Malformed input	Behavior
Unrecognized code	Degrades to UNKNOWN, original text kept on message
Missing code	Degrades to UNKNOWN, message preserved
Empty reason list	Yields one UNKNOWN reason so first stays safe
Unknown extra keys	Ignored
success: true alongside an error	Error wins; success is derived, never trusted

That matters because the enum is closed, so a wrapper built against an older SDK will eventually meet a code it can't name — a strict valueOf would throw at exactly that moment.

Self-review

Five defects found on a fresh adversarial pass, all fixed here:

• KDoc documented a wire shape that no longer exists. OneSignalResult's class doc still showed the nested error.error envelope from before the list was flattened. This file is what a wrapper author reads to build their parser, so a wrong doc is a defect in the deliverable.
• The non-empty invariant was documented but unenforced. error is documented "Never empty" and first as always safe, but only the factories guarded it — a direct constructor call did not.
• The invariant could still be defeated after construction. Found while writing up the previous item: the list was aliased rather than copied, so a caller holding a MutableList could clear it after require passed. Now a defensive toList().
• Exception text appended a bare null. A Detail with no message rendered as "STORAGE_LOCKED: null" in getOrThrow's stack trace.
• Interface declared in the wrong file. OneSignalResultData lived in OneSignalResult.kt while OneSignalResultData.kt held only the payloads.

One open question

Every constructor here is internal. That correctly stops customers fabricating SDK results in production, but it also stops them unit-testing their own error-handling branches — there's no supported way to build a failed OneSignalResult in a customer's test suite, and wrapper SDKs hit the same wall unless they live in this Gradle module. Worth deciding now, since a testing entry point is additive and cheap to add later but awkward to discover after adoption.

Not in this PR

Nothing populates BACKEND_ERROR or backendCode. That detail isn't reachable from the entry points yet; SDK-4989 is what carries it up. This PR only defines where it will go.

Test plan

• :OneSignal:core:testDebugUnitTest — full core suite green
• :OneSignal:core:apiCheck — .api diff shows ErrorCode/ErrorSource as java/lang/Enum
• :OneSignal:core:detekt and spotlessApply
• Round-trip coverage for success, failure, and empty payloads
• Degradation coverage for an unrecognized code, a missing code, and an empty reason list
• Constructor rejects an empty reason list

Made with Cursor

[github-actions]

📊 Diff Coverage Report

Diff 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

• OneSignalError.kt: 27/29 touched executable lines (93.1%) (224 touched lines in diff)
• OneSignalResult.kt: 20/21 touched executable lines (95.2%) (149 touched lines in diff)
• OneSignalResultData.kt: 9/17 touched executable lines (52.9%) (93 touched lines in diff)
  • 8 uncovered touched lines in this file

Overall (aggregate gate)

56/67 touched executable lines covered (83.6% — requires ≥ 80%)

Per-file detail (informational; gate is aggregate above):

• OneSignalResultData.kt: 52.9% (8 uncovered touched lines)

📥 View workflow run

[abdulraqeeb33]

i have added this an example, i will get rid of it when i integrate it with the login api

[fadi-george]

i thought were avoiding error codes

[abdulraqeeb33]

the thought was, there are existing error status that we want to return, mostly on the client side. I was trying to figure out a way where we can differentiate between client (we know of) and server codes that we are unaware of. And this was a better representation of it that seems to be scalable.

[fadi-george]

Ran this through a couple of adversarial passes. Design reads well, but a few things in the parsing half are worth fixing before the wire contract is locked in.

fromMap picks the error branch with as? List<...>, which doubles as the predicate. Anything present but not a kotlin.collections.List (a bare map, or a JSONArray, which doesn't implement java.util.List) silently falls through to the success branch and yields LoginData("", "") with isSuccess == true. The reverse direction is guarded but this one isn't, and org.json is the natural parse on the bridge side.

Same cast is erased at the element level, so error = listOf("oops") gets past it and throws ClassCastException inside Detail.fromMap. The suppression is what hides it. Both cases contradict the never-throws table.

getError() hands back a mutable ArrayList. The defensive copy stops the caller mutating the input list, but not the copy, so from Java getError().clear() then getFirst() throws and the require is bypassed. Collections.unmodifiableList closes it.

source goes out on the wire but fromMap never reads it back, and unknown codes degrade to UNKNOWN which is hardcoded CLIENT. So a backend failure comes back attributed to the client in exactly the forward compat case the degradation is for. For a schema that has to stay identical across SDKs, source is write only right now and another SDK can't tell whether it's authoritative.

On the open question about internal constructors: they don't actually stop fabrication. internal on a constructor is metadata only, it emits as JVM public, so new OneSignalResult<>(null, null) compiles from Java. BCV also filters internal out of the dump, so apiCheck can't see that surface at all. Worth deciding deliberately rather than leaving an accidental entry point only Java can reach.

Two smaller ones: OneSignalResultData is publicly implementable so any member added later is a break, sealed would close it at no cost. And OneSignalResult doesn't enforce its own either/or invariant, with isSuccess keyed off error and getOrThrow off data, so the two disagree whenever it's violated.

[abdulraqeeb33]

Thanks Fadi — this was a genuinely useful pass, and all seven are fixed in f3dd107c3.

The parsing ones were the real finds. You were right that the cast was doubling as the predicate, and right that the KDoc directly above it claims to guard the very thing it was letting through. Presence and shape are now separate concerns: fromMap only asks whether the key is there, and fromWire deals with whatever arrived. Rather than just rejecting non-List shapes, JSONArray and JSONObject are now converted through our existing org.json helpers, so a bridge payload keeps its structured codes instead of degrading. The element-level erasure is gone with it — Detail.fromMap takes Map<*, *> now, so the unchecked cast is removed rather than suppressed.

Worth flagging that data had the identical defect you found in error, which neither of us mentioned: a JSONObject payload silently produced LoginData("", "") with isSuccess == true. Same fix, and it's the one I'd have least liked to ship.

source is now its own field on Detail, defaulting to code.source but read from the wire when present, so your RATE_LIMITED/BACKEND/429 case degrades to UNKNOWN while keeping BACKEND and the backend code instead of inverting to CLIENT. getError() is wrapped in unmodifiableList. OneSignalResultData is sealed.

On the constructors — you're right that internal is metadata only, and I went to private rather than trying to make internal mean something. Worth noting the invariant does most of that work anyway: with require((data == null) != (error == null)) in the constructor, the disagreement between isSuccess and getOrThrow() you identified can't exist regardless of who calls it, which turns the leak from a correctness hole into an API-surface question.

One thing your review surfaced indirectly: our tests couldn't have caught any of this. Every round-trip test fed toMap()'s own output back into fromMap(), so the parser only ever saw the types the serializer emits, and the malformed-input tests were all malformed in well-typed ways — right containers, wrong keys. The suite tested wrong values and never wrong shapes. The test named "error presence wins over a contradictory success flag" asserts exactly the property your first finding violates, and passed only because its error happened to be a proper List. That's fixed too: the new tests use real JSONArray/JSONObject instances and non-map elements, and each one fails against the old code.

Two I'd like your read on rather than deciding unilaterally. sealed closes the interface as you suggested, but it also means payloads must live in the same package and module — so if a suspend API on notifications or IAM ever needs its own payload type, it can't implement this. Fine for now, worth knowing. And separately, LoginData.toString() prints the external ID, which is usually a customer email — that's a logging leak that exists today and isn't something this PR introduced, so I'd rather fix it deliberately than fold it in here. Happy to do either.

[fadi-george]

The earlier issues are addressed, but 2 confirmed cases remain:

1. Missing, null, empty, or wrongly typed LoginData fields still produce a successful LoginData("", ""). Required identity fields should fail parsing rather than fabricate a blank success.
2. JSONObject.NULL under error is non-null, so it is parsed as an UNKNOWN error with message "null", turning a valid success into a failure.
   Once these are covered, I’m good to approve.