From a76901e77c692752fb8e7ba8234274fc04b83ac0 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Fri, 7 Aug 2026 10:51:10 -0500 Subject: [PATCH 1/4] feat: [SDK-4988] add OneSignalResult, OneSignalError, and per-method payloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pure addition, wired to nothing, so the type design can be reviewed before any API changes shape around it. OneSignalResult is the envelope: isSuccess, the payload, the error, plus toMap/fromMap projections onto the cross-SDK wire schema. OneSignalError carries a list of Detail — one request can fail for several reasons at once — plus the originating Throwable. ErrorCode is an enum rather than a sealed hierarchy so Java gets a native `switch` and the wrapper bridges get a `name()` marshal; each constant carries an ErrorSource saying whether the SDK produced it locally or the backend returned it. Backend catalog codes stay a raw Int on Detail so the backend can add them without an SDK release gating recognition. Detail is nested to keep a top-level `Error` from shadowing kotlin.Error. Unrecognized codes degrade to UNKNOWN with the original text preserved, so a wrapper built against an older SDK survives a newer producer. Co-authored-by: Cursor --- OneSignalSDK/onesignal/core/api/core.api | 76 ++++++ .../main/java/com/onesignal/OneSignalError.kt | 160 ++++++++++++ .../java/com/onesignal/OneSignalResult.kt | 113 ++++++++ .../java/com/onesignal/OneSignalResultData.kt | 92 +++++++ .../com/onesignal/OneSignalResultTests.kt | 247 ++++++++++++++++++ 5 files changed, 688 insertions(+) create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt diff --git a/OneSignalSDK/onesignal/core/api/core.api b/OneSignalSDK/onesignal/core/api/core.api index ccdf99ba5..3b8325644 100644 --- a/OneSignalSDK/onesignal/core/api/core.api +++ b/OneSignalSDK/onesignal/core/api/core.api @@ -13,6 +13,26 @@ public final class com/onesignal/ContinueResult { public final fun isSuccess ()Z } +public final class com/onesignal/ErrorCode : java/lang/Enum { + public static final field BACKEND_ERROR Lcom/onesignal/ErrorCode; + public static final field INVALID_ARGUMENT Lcom/onesignal/ErrorCode; + public static final field NOT_INITIALIZED Lcom/onesignal/ErrorCode; + public static final field STORAGE_LOCKED Lcom/onesignal/ErrorCode; + public static final field UNKNOWN Lcom/onesignal/ErrorCode; + public static fun getEntries ()Lkotlin/enums/EnumEntries; + public final fun getSource ()Lcom/onesignal/ErrorSource; + public static fun valueOf (Ljava/lang/String;)Lcom/onesignal/ErrorCode; + public static fun values ()[Lcom/onesignal/ErrorCode; +} + +public final class com/onesignal/ErrorSource : java/lang/Enum { + public static final field BACKEND Lcom/onesignal/ErrorSource; + public static final field CLIENT Lcom/onesignal/ErrorSource; + public static fun getEntries ()Lkotlin/enums/EnumEntries; + public static fun valueOf (Ljava/lang/String;)Lcom/onesignal/ErrorSource; + public static fun values ()[Lcom/onesignal/ErrorSource; +} + public abstract interface class com/onesignal/IOneSignal { public abstract fun addUserJwtInvalidatedListener (Lcom/onesignal/IUserJwtInvalidatedListener;)V public abstract fun getConsentGiven ()Z @@ -64,6 +84,23 @@ public abstract interface class com/onesignal/IUserJwtInvalidatedListener { public abstract fun onUserJwtInvalidated (Lcom/onesignal/UserJwtInvalidatedEvent;)V } +public final class com/onesignal/InitData : com/onesignal/OneSignalResultData { + public fun toMap ()Ljava/util/Map; + public fun toString ()Ljava/lang/String; +} + +public final class com/onesignal/LoginData : com/onesignal/OneSignalResultData { + public final fun getExternalId ()Ljava/lang/String; + public final fun getOnesignalId ()Ljava/lang/String; + public fun toMap ()Ljava/util/Map; + public fun toString ()Ljava/lang/String; +} + +public final class com/onesignal/LogoutData : com/onesignal/OneSignalResultData { + public fun toMap ()Ljava/util/Map; + public fun toString ()Ljava/lang/String; +} + public final class com/onesignal/OneSignal { public static final field INSTANCE Lcom/onesignal/OneSignal; public static final fun addUserJwtInvalidatedListener (Lcom/onesignal/IUserJwtInvalidatedListener;)V @@ -109,12 +146,51 @@ public final class com/onesignal/OneSignal { public static final fun updateUserJwtSuspend (Ljava/lang/String;Ljava/lang/String;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; } +public final class com/onesignal/OneSignalError { + public final fun getCause ()Ljava/lang/Throwable; + public final fun getError ()Ljava/util/List; + public final fun getFirst ()Lcom/onesignal/OneSignalError$Detail; + public final fun toList ()Ljava/util/List; + public fun toString ()Ljava/lang/String; +} + +public final class com/onesignal/OneSignalError$Detail { + public final fun getBackendCode ()Ljava/lang/Integer; + public final fun getCode ()Lcom/onesignal/ErrorCode; + public final fun getMessage ()Ljava/lang/String; + public final fun toMap ()Ljava/util/Map; + public fun toString ()Ljava/lang/String; +} + +public final class com/onesignal/OneSignalException : java/lang/Exception { + public final fun getError ()Lcom/onesignal/OneSignalError; +} + +public final class com/onesignal/OneSignalResult { + public final fun getData ()Lcom/onesignal/OneSignalResultData; + public final fun getError ()Lcom/onesignal/OneSignalError; + public final fun getOrNull ()Lcom/onesignal/OneSignalResultData; + public final fun getOrThrow ()Lcom/onesignal/OneSignalResultData; + public final fun isSuccess ()Z + public final fun toMap ()Ljava/util/Map; + public fun toString ()Ljava/lang/String; +} + +public abstract interface class com/onesignal/OneSignalResultData { + public abstract fun toMap ()Ljava/util/Map; +} + public final class com/onesignal/SyncJobService : android/app/job/JobService { public fun ()V public fun onStartJob (Landroid/app/job/JobParameters;)Z public fun onStopJob (Landroid/app/job/JobParameters;)Z } +public final class com/onesignal/UpdateUserJwtData : com/onesignal/OneSignalResultData { + public fun toMap ()Ljava/util/Map; + public fun toString ()Ljava/lang/String; +} + public final class com/onesignal/UserJwtInvalidatedEvent { public fun (Ljava/lang/String;)V public final fun getExternalId ()Ljava/lang/String; diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt new file mode 100644 index 000000000..58349fa1a --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt @@ -0,0 +1,160 @@ +package com.onesignal + +/** Whether the SDK produced a failure locally or OneSignal's backend returned it. */ +enum class ErrorSource { + CLIENT, + BACKEND, +} + +/** + * The catalog of failure codes shared by every OneSignal SDK. + * + * An enum rather than a sealed hierarchy so that Java callers get a native `switch` and the + * wrapper bridges get a trivial name-to-string marshal. The backend half of the catalog is + * deliberately *not* modelled here — see [BACKEND_ERROR]. + */ +enum class ErrorCode(val source: ErrorSource) { + /** [IOneSignal.initWithContextSuspend] has not been called. */ + NOT_INITIALIZED(ErrorSource.CLIENT), + + /** + * Device storage was locked, so the SDK could not read or write its own preferences. + * Transient: the same call generally succeeds once the device is unlocked. + */ + STORAGE_LOCKED(ErrorSource.CLIENT), + + /** A caller-supplied argument failed validation before any request was made. */ + INVALID_ARGUMENT(ErrorSource.CLIENT), + + /** OneSignal rejected the request. The catalog code is on [OneSignalError.Detail.backendCode]. */ + BACKEND_ERROR(ErrorSource.BACKEND), + + /** No more specific code applies. Callers should surface [OneSignalError.Detail.message]. */ + UNKNOWN(ErrorSource.CLIENT), +} + +/** + * Describes why a OneSignal call failed. + * + * One request can fail for several reasons at once, so [error] is a list of [Detail]. Everything + * the SDK raises locally has exactly one reason, which [first] reads without the indexing + * ceremony. + * + * On the wire this is the list itself, sitting under the envelope's `error` key: + * + * ```json + * { "success": false, "data": null, + * "error": [ { "code": "STORAGE_LOCKED", "source": "CLIENT", "backendCode": null, "message": "..." } ] } + * ``` + */ +class OneSignalError internal constructor( + error: List, + /** + * The throwable behind the failure, when there was one. + * + * Deliberately absent from [toList]: a stack trace cannot cross the wrapper bridges, and the + * wire schema has to stay identical across every SDK. This exists so that native Kotlin and + * Java callers do not lose the stack when the suspend APIs report a failure instead of + * throwing it. + */ + val cause: Throwable? = null, +) { + /** + * Why the call failed. Never empty. + * + * Copied rather than aliased so that a caller holding the original list cannot empty it + * afterwards and leave [first] throwing. + */ + val error: List = error.toList() + + init { + // [first] is documented as always safe to read, and the wire projection of an empty error + // would claim failure while explaining nothing. Both factories guard this; the check is + // here so a future caller of the constructor cannot quietly break the invariant. + require(this.error.isNotEmpty()) { "OneSignalError requires at least one Detail." } + } + + /** + * A single reason a call failed. + * + * Nested rather than top-level so the name cannot collide with `kotlin.Error`, which is + * auto-imported everywhere, or shadow `java.lang.Error` in a Java file that imports it. + */ + class Detail internal constructor( + /** A stable code, safe to branch on. Never localized. */ + val code: ErrorCode, + /** + * The backend's catalog code, present only when [code] is [ErrorCode.BACKEND_ERROR]. + * + * Left as a raw number on purpose: the backend adds codes on its own schedule, and an SDK + * release must not be the thing that unblocks recognizing one. + */ + val backendCode: Int? = null, + /** A human-readable description intended for logs and diagnostics, not for end users. */ + val message: String? = null, + ) { + /** Projects this reason onto the cross-SDK wire shape consumed by the wrapper bridges. */ + fun toMap(): Map = + mapOf( + KEY_CODE to code.name, + KEY_SOURCE to code.source.name, + KEY_BACKEND_CODE to backendCode, + KEY_MESSAGE to message, + ) + + override fun toString(): String = "Detail(code=$code, backendCode=$backendCode, message=$message)" + + internal companion object { + // Private because `const val` in an internal companion still compiles to a public + // static field, which would leak the wire keys into the customer-facing API surface. + private const val KEY_CODE = "code" + private const val KEY_SOURCE = "source" + private const val KEY_BACKEND_CODE = "backendCode" + private const val KEY_MESSAGE = "message" + + /** + * Rebuilds a reason from its wire shape. + * + * An unrecognized code degrades to [ErrorCode.UNKNOWN] rather than throwing, so a + * wrapper built against an older SDK survives a newer producer emitting a code it has + * never heard of. The original text is preserved on [message] either way. + */ + fun fromMap(map: Map): Detail = + Detail( + code = codeOf(map[KEY_CODE] as? String), + backendCode = (map[KEY_BACKEND_CODE] as? Number)?.toInt(), + message = map[KEY_MESSAGE] as? String, + ) + + private fun codeOf(name: String?): ErrorCode = ErrorCode.entries.firstOrNull { it.name == name } ?: ErrorCode.UNKNOWN + } + } + + /** The first reason, which is the only one for every failure the SDK raises locally. */ + val first: Detail + get() = error.first() + + /** Projects this error onto the cross-SDK wire shape consumed by the wrapper bridges. */ + fun toList(): List> = error.map { it.toMap() } + + override fun toString(): String = "OneSignalError(error=$error)" + + internal companion object { + /** Builds a single-reason error, which is the shape of everything the SDK raises locally. */ + fun of( + code: ErrorCode, + message: String? = null, + backendCode: Int? = null, + cause: Throwable? = null, + ): OneSignalError = OneSignalError(listOf(Detail(code, backendCode, message)), cause) + + /** + * Rebuilds an error from its wire shape. A payload carrying no recognizable reason still + * yields a usable error rather than an empty list, so [first] is always safe. + */ + fun fromList(reasons: List>): OneSignalError = + OneSignalError( + reasons.map { Detail.fromMap(it) }.takeIf { it.isNotEmpty() } ?: listOf(Detail(ErrorCode.UNKNOWN)), + ) + } +} diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt new file mode 100644 index 000000000..deed236e5 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt @@ -0,0 +1,113 @@ +package com.onesignal + +/** + * The outcome of an asynchronous OneSignal call: either a [data] payload or an [error], never both + * and never neither. + * + * Every SDK returns the same envelope, so a wrapper can handle results uniformly regardless of the + * platform underneath it: + * + * ```json + * { "success": true, "data": { }, "error": null } + * { "success": false, "data": null, "error": [ { "code": "STORAGE_LOCKED", ... } ] } + * ``` + * + * The presence of [error] is what defines the outcome; [isSuccess] and the wire-level `success` + * flag are both derived from it, so the two can never disagree. + * + * From Kotlin: + * ```kotlin + * val result = OneSignal.login("user-123") + * if (result.isSuccess) println(result.data?.onesignalId) else println(result.error?.first?.code) + * ``` + * + * From Java the generated accessors read naturally: + * ```java + * if (result.isSuccess()) { result.getData(); } else { result.getError(); } + * ``` + */ +class OneSignalResult internal constructor( + /** The payload on success, `null` on failure. */ + val data: T?, + /** The failure detail on failure, `null` on success. */ + val error: OneSignalError?, +) { + /** `true` when the call completed successfully. Equivalent to `error == null`. */ + val isSuccess: Boolean + get() = error == null + + /** Kotlin-idiomatic alias for [data]. */ + fun getOrNull(): T? = data + + /** + * Returns the payload, or throws [OneSignalException] when the call failed. Use this only where + * a failure genuinely cannot be handled locally. + */ + fun getOrThrow(): T = data ?: throw OneSignalException(error ?: unexpectedMissingError()) + + /** Projects the envelope onto the cross-SDK wire shape consumed by the wrapper bridges. */ + fun toMap(): Map = + mapOf( + KEY_SUCCESS to isSuccess, + KEY_DATA to data?.toMap(), + KEY_ERROR to error?.toList(), + ) + + override fun toString(): String = if (isSuccess) "OneSignalResult(success, data=$data)" else "OneSignalResult(failure, error=$error)" + + private fun unexpectedMissingError() = OneSignalError.of(ErrorCode.UNKNOWN, "Result carried neither data nor error.") + + internal companion object { + // Private because `const val` in an internal companion still compiles to a public static + // field, which would leak the wire keys into the customer-facing API surface. + private const val KEY_SUCCESS = "success" + private const val KEY_DATA = "data" + private const val KEY_ERROR = "error" + + fun success(data: T): OneSignalResult = OneSignalResult(data, null) + + fun failure(error: OneSignalError): OneSignalResult = OneSignalResult(null, error) + + fun failure( + code: ErrorCode, + message: String? = null, + backendCode: Int? = null, + cause: Throwable? = null, + ): OneSignalResult = failure(OneSignalError.of(code, message, backendCode, cause)) + + /** + * Rebuilds an envelope from its wire shape, delegating payload parsing to [dataParser]. + * + * The incoming `success` flag is deliberately ignored: [error] is the single source of + * truth, which keeps a malformed producer from yielding a result that claims success while + * carrying an error. Unrecognized keys are ignored so a newer producer can add fields + * without breaking an older consumer. + */ + @Suppress("UNCHECKED_CAST") + fun fromMap( + map: Map, + dataParser: (Map) -> T, + ): OneSignalResult { + val reasons = map[KEY_ERROR] as? List> + if (reasons != null) { + return failure(OneSignalError.fromList(reasons)) + } + + val dataMap = map[KEY_DATA] as? Map ?: emptyMap() + return success(dataParser(dataMap)) + } + } +} + +/** Thrown by [OneSignalResult.getOrThrow] when the underlying call failed. */ +class OneSignalException internal constructor( + /** The failure detail that caused this exception. */ + val error: OneSignalError, +) : Exception(describe(error), error.cause) + +// A Detail carries no message when the code says everything, so appending a bare "null" to the +// exception text would only add noise to the stack trace. +private fun describe(error: OneSignalError): String = + error.error.joinToString("; ") { detail -> + if (detail.message == null) detail.code.name else "${detail.code}: ${detail.message}" + } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt new file mode 100644 index 000000000..73a9b5032 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt @@ -0,0 +1,92 @@ +package com.onesignal + +/** + * Implemented by every payload that can travel inside a [OneSignalResult]. + * + * The contract exists so the envelope can be projected onto the wire shape without knowing which + * payload it is carrying. It is not intended to be implemented outside the SDK. + */ +interface OneSignalResultData { + /** Projects this payload onto the cross-SDK wire shape consumed by the wrapper bridges. */ + fun toMap(): Map +} + +/* + * The payloads themselves. + * + * Several of these start with no fields. That is intentional, and it is why none of them is a + * `data class` or an `object`: a regular class with an internal constructor can gain nullable or + * defaulted fields later without changing any signature, without a singleton blocking per-call + * state, and without silently altering generated equals/hashCode/toString/copy behavior that a + * customer may have come to depend on. + * + * Field names here are also the wire keys, so they are additive-only: never renamed, never removed. + */ + +/** The payload returned by a successful login. */ +class LoginData internal constructor( + /** The OneSignal ID the external ID is now associated with. */ + val onesignalId: String, + /** The external ID that was logged in. */ + val externalId: String, +) : OneSignalResultData { + override fun toMap(): Map = + mapOf( + KEY_ONESIGNAL_ID to onesignalId, + KEY_EXTERNAL_ID to externalId, + ) + + override fun toString(): String = "LoginData(onesignalId=$onesignalId, externalId=$externalId)" + + internal companion object { + // Private because `const val` in an internal companion still compiles to a public static + // field, which would leak the wire keys into the customer-facing API surface. + private const val KEY_ONESIGNAL_ID = "onesignalId" + private const val KEY_EXTERNAL_ID = "externalId" + + fun fromMap(map: Map): LoginData = + LoginData( + onesignalId = map[KEY_ONESIGNAL_ID] as? String ?: "", + externalId = map[KEY_EXTERNAL_ID] as? String ?: "", + ) + } +} + +/** The payload returned by a successful logout. Carries no fields yet. */ +class LogoutData internal constructor() : OneSignalResultData { + override fun toMap(): Map = emptyMap() + + override fun toString(): String = "LogoutData()" + + internal companion object { + fun fromMap( + @Suppress("UNUSED_PARAMETER") map: Map, + ): LogoutData = LogoutData() + } +} + +/** The payload returned by a successful user JWT update. Carries no fields yet. */ +class UpdateUserJwtData internal constructor() : OneSignalResultData { + override fun toMap(): Map = emptyMap() + + override fun toString(): String = "UpdateUserJwtData()" + + internal companion object { + fun fromMap( + @Suppress("UNUSED_PARAMETER") map: Map, + ): UpdateUserJwtData = UpdateUserJwtData() + } +} + +/** The payload returned by a successful initialization. Carries no fields yet. */ +class InitData internal constructor() : OneSignalResultData { + override fun toMap(): Map = emptyMap() + + override fun toString(): String = "InitData()" + + internal companion object { + fun fromMap( + @Suppress("UNUSED_PARAMETER") map: Map, + ): InitData = InitData() + } +} diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt new file mode 100644 index 000000000..3725a7324 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt @@ -0,0 +1,247 @@ +package com.onesignal + +import io.kotest.assertions.throwables.shouldThrow +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.booleans.shouldBeFalse +import io.kotest.matchers.booleans.shouldBeTrue +import io.kotest.matchers.nulls.shouldBeNull +import io.kotest.matchers.nulls.shouldNotBeNull +import io.kotest.matchers.shouldBe +import io.kotest.matchers.types.shouldBeInstanceOf + +class OneSignalResultTests : FunSpec({ + + test("success carries data and no error") { + val result = OneSignalResult.success(LoginData("os-1", "ext-1")) + + result.isSuccess.shouldBeTrue() + result.error.shouldBeNull() + result.data.shouldNotBeNull() + result.data!!.onesignalId shouldBe "os-1" + result.getOrNull().shouldNotBeNull() + result.getOrThrow().externalId shouldBe "ext-1" + } + + test("failure carries error and no data") { + val result = OneSignalResult.failure(ErrorCode.INVALID_ARGUMENT, "no app ID") + + result.isSuccess.shouldBeFalse() + result.data.shouldBeNull() + result.getOrNull().shouldBeNull() + result.error.shouldNotBeNull() + result.error!!.first.code shouldBe ErrorCode.INVALID_ARGUMENT + result.error!!.first.message shouldBe "no app ID" + } + + test("a client code is distinguishable from a backend one without inspecting the message") { + val client = OneSignalResult.failure(ErrorCode.STORAGE_LOCKED, "device locked") + val backend = OneSignalResult.failure(ErrorCode.BACKEND_ERROR, "Invalid API Key", backendCode = 100) + + client.error!!.first.code.source shouldBe ErrorSource.CLIENT + client.error!!.first.backendCode.shouldBeNull() + + backend.error!!.first.code.source shouldBe ErrorSource.BACKEND + backend.error!!.first.backendCode shouldBe 100 + } + + test("an error can carry several reasons at once") { + val error = + OneSignalError( + listOf( + OneSignalError.Detail(ErrorCode.BACKEND_ERROR, 100, "Invalid API Key"), + OneSignalError.Detail(ErrorCode.BACKEND_ERROR, 144, "Invalid external ID"), + ), + ) + + error.error.size shouldBe 2 + error.first.backendCode shouldBe 100 + error.toList().map { it["backendCode"] } shouldBe listOf(100, 144) + } + + // first is documented as always safe to read, so the constructor has to refuse the one input + // that would make it throw. + test("an error cannot be built with no reasons") { + shouldThrow { OneSignalError(emptyList()) } + } + + test("a reason with no message keeps the exception text free of a bare null") { + val result = OneSignalResult.failure(ErrorCode.STORAGE_LOCKED) + + val thrown = + runCatching { result.getOrThrow() } + .exceptionOrNull() + .shouldBeInstanceOf() + + thrown.message shouldBe "STORAGE_LOCKED" + } + + test("getOrThrow surfaces the error and keeps the cause attached") { + val boom = IllegalStateException("boom") + val result = OneSignalResult.failure(ErrorCode.UNKNOWN, "offline", cause = boom) + + val thrown = + runCatching { result.getOrThrow() } + .exceptionOrNull() + .shouldBeInstanceOf() + + thrown.error.first.code shouldBe ErrorCode.UNKNOWN + thrown.message shouldBe "UNKNOWN: offline" + thrown.cause shouldBe boom + } + + test("the cause stays off the wire so every SDK serializes the same shape") { + val error = OneSignalError.of(ErrorCode.UNKNOWN, "boom", cause = IllegalStateException("boom")) + + error.cause.shouldNotBeNull() + error.toList().single().keys shouldBe setOf("code", "source", "backendCode", "message") + } + + test("success projects onto the wire envelope") { + val map = OneSignalResult.success(LoginData("os-1", "ext-1")).toMap() + + map shouldBe + mapOf( + "success" to true, + "data" to mapOf("onesignalId" to "os-1", "externalId" to "ext-1"), + "error" to null, + ) + } + + test("failure projects onto the wire envelope") { + val map = OneSignalResult.failure(ErrorCode.STORAGE_LOCKED, "device locked").toMap() + + map shouldBe + mapOf( + "success" to false, + "data" to null, + "error" to + listOf( + mapOf( + "code" to "STORAGE_LOCKED", + "source" to "CLIENT", + "backendCode" to null, + "message" to "device locked", + ), + ), + ) + } + + test("an empty payload still serializes as a present, empty data object") { + val map = OneSignalResult.success(InitData()).toMap() + + map["success"] shouldBe true + map["error"].shouldBeNull() + map.containsKey("data").shouldBeTrue() + map["data"] shouldBe emptyMap() + } + + test("success round-trips through the wire shape") { + val original = OneSignalResult.success(LoginData("os-1", "ext-1")) + + val restored = OneSignalResult.fromMap(original.toMap(), LoginData::fromMap) + + restored.isSuccess.shouldBeTrue() + restored.toMap() shouldBe original.toMap() + } + + test("failure round-trips through the wire shape") { + val original = OneSignalResult.failure(ErrorCode.BACKEND_ERROR, "already linked", backendCode = 409) + + val restored = OneSignalResult.fromMap(original.toMap(), LoginData::fromMap) + + restored.isSuccess.shouldBeFalse() + restored.toMap() shouldBe original.toMap() + } + + test("an empty payload round-trips through the wire shape") { + val original = OneSignalResult.success(InitData()) + + val restored = OneSignalResult.fromMap(original.toMap(), InitData::fromMap) + + restored.isSuccess.shouldBeTrue() + restored.toMap() shouldBe original.toMap() + } + + test("unknown envelope and payload fields are ignored rather than throwing") { + val fromNewerProducer = + mapOf( + "success" to true, + "data" to + mapOf( + "onesignalId" to "os-1", + "externalId" to "ext-1", + "subscriptionId" to "sub-9", + ), + "error" to null, + "traceId" to "abc-123", + ) + + val restored = OneSignalResult.fromMap(fromNewerProducer, LoginData::fromMap) + + restored.isSuccess.shouldBeTrue() + restored.data!!.onesignalId shouldBe "os-1" + restored.toMap() shouldBe + mapOf( + "success" to true, + "data" to mapOf("onesignalId" to "os-1", "externalId" to "ext-1"), + "error" to null, + ) + } + + // The enum is closed, so a wrapper running against a newer producer will eventually meet a + // code it cannot name. It has to degrade rather than throw out of valueOf. + test("an unrecognized code degrades to UNKNOWN with the message preserved") { + val fromNewerProducer = + mapOf( + "success" to false, + "data" to null, + "error" to listOf(mapOf("code" to "RATE_LIMITED", "message" to "slow down", "retryAfterSeconds" to 30)), + ) + + val restored = OneSignalResult.fromMap(fromNewerProducer, LoginData::fromMap) + + restored.isSuccess.shouldBeFalse() + restored.error!!.first.code shouldBe ErrorCode.UNKNOWN + restored.error!!.first.message shouldBe "slow down" + } + + test("a malformed error missing its code degrades to unknown instead of throwing") { + val restored = + OneSignalResult.fromMap( + mapOf("success" to false, "data" to null, "error" to listOf(mapOf("message" to "something broke"))), + LoginData::fromMap, + ) + + restored.isSuccess.shouldBeFalse() + restored.error!!.first.code shouldBe ErrorCode.UNKNOWN + restored.error!!.first.message shouldBe "something broke" + } + + // first is documented as always safe to read, so an error list that arrives empty still has + // to produce one reason rather than blowing up at the call site. + test("an empty reason list still yields a readable error") { + val restored = + OneSignalResult.fromMap( + mapOf("success" to false, "data" to null, "error" to emptyList>()), + LoginData::fromMap, + ) + + restored.isSuccess.shouldBeFalse() + restored.error!!.first.code shouldBe ErrorCode.UNKNOWN + } + + test("error presence wins over a contradictory success flag") { + val contradictory = + mapOf( + "success" to true, + "data" to mapOf("onesignalId" to "os-1", "externalId" to "ext-1"), + "error" to listOf(mapOf("code" to "STORAGE_LOCKED", "message" to "device locked")), + ) + + val restored = OneSignalResult.fromMap(contradictory, LoginData::fromMap) + + restored.isSuccess.shouldBeFalse() + restored.data.shouldBeNull() + restored.error!!.first.code shouldBe ErrorCode.STORAGE_LOCKED + } +}) From d006b4712c3c21924b558acc502911dca98bb451 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Tue, 11 Aug 2026 11:37:13 -0500 Subject: [PATCH 2/4] fix: close the shape, invariant, mutability, and attribution gaps in the result model Four correctness fixes on the OneSignalResult/OneSignalError wire model, each covered by a test that fails without it: - fromMap tested `error` for presence with a cast that doubled as the predicate, so an error the parser could not type read as no error at all. A JSONArray, which is what a bridge naturally parses with and is not a java.util.List, turned a failure into a blank success; a list holding a non-map reason threw ClassCastException out of a model whose premise is that it never throws. The shape is now read defensively and an unreadable reason keeps its own text. - The envelope carried no guard, so it could be built with neither data nor error or with both, leaving isSuccess and getOrThrow disagreeing. A require now enforces exactly one, mirroring the guard in OneSignalError. - getError() returned a copy, which stopped the caller emptying the list they passed in but not the copy itself. It is now unmodifiable, so `first` stays safe to read as documented. - Detail re-derived source from code on demand. A code newer than this SDK degrades to UNKNOWN, so a backend failure arrived attributed to the client while still carrying its backend code. Detail now carries its own source, defaulting to the one its code implies and read from the wire when present. Also closes the test blind spot behind these: every round-trip case fed toMap()'s own output back into fromMap(), so the parser was only ever shown shapes it had just produced. --- OneSignalSDK/onesignal/core/api/core.api | 1 + .../main/java/com/onesignal/OneSignalError.kt | 66 +++++++-- .../java/com/onesignal/OneSignalResult.kt | 19 ++- .../com/onesignal/OneSignalResultTests.kt | 138 +++++++++++++++++- 4 files changed, 202 insertions(+), 22 deletions(-) diff --git a/OneSignalSDK/onesignal/core/api/core.api b/OneSignalSDK/onesignal/core/api/core.api index 3b8325644..0e51dd3dd 100644 --- a/OneSignalSDK/onesignal/core/api/core.api +++ b/OneSignalSDK/onesignal/core/api/core.api @@ -158,6 +158,7 @@ public final class com/onesignal/OneSignalError$Detail { public final fun getBackendCode ()Ljava/lang/Integer; public final fun getCode ()Lcom/onesignal/ErrorCode; public final fun getMessage ()Ljava/lang/String; + public final fun getSource ()Lcom/onesignal/ErrorSource; public final fun toMap ()Ljava/util/Map; public fun toString ()Ljava/lang/String; } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt index 58349fa1a..2a1ec0da0 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt @@ -1,5 +1,7 @@ package com.onesignal +import java.util.Collections + /** Whether the SDK produced a failure locally or OneSignal's backend returned it. */ enum class ErrorSource { CLIENT, @@ -62,10 +64,11 @@ class OneSignalError internal constructor( /** * Why the call failed. Never empty. * - * Copied rather than aliased so that a caller holding the original list cannot empty it - * afterwards and leave [first] throwing. + * Copied so a caller holding the original list cannot empty it afterwards, and unmodifiable so + * the copy itself cannot be emptied either. Java sees a plain `List` and `clear()` is one + * keystroke away from `get()`; both would leave [first] throwing. */ - val error: List = error.toList() + val error: List = Collections.unmodifiableList(error.toList()) init { // [first] is documented as always safe to read, and the wire projection of an empty error @@ -92,17 +95,26 @@ class OneSignalError internal constructor( val backendCode: Int? = null, /** A human-readable description intended for logs and diagnostics, not for end users. */ val message: String? = null, + /** + * Who the failure came from. + * + * Carried rather than derived from [code] on demand, because a code this SDK does not + * recognize degrades to [ErrorCode.UNKNOWN] and re-deriving from that would report a + * backend failure as a client one. Defaults to the source [code] implies, which is right + * for everything the SDK raises locally. + */ + val source: ErrorSource = code.source, ) { /** Projects this reason onto the cross-SDK wire shape consumed by the wrapper bridges. */ fun toMap(): Map = mapOf( KEY_CODE to code.name, - KEY_SOURCE to code.source.name, + KEY_SOURCE to source.name, KEY_BACKEND_CODE to backendCode, KEY_MESSAGE to message, ) - override fun toString(): String = "Detail(code=$code, backendCode=$backendCode, message=$message)" + override fun toString(): String = "Detail(code=$code, source=$source, backendCode=$backendCode, message=$message)" internal companion object { // Private because `const val` in an internal companion still compiles to a public @@ -115,18 +127,28 @@ class OneSignalError internal constructor( /** * Rebuilds a reason from its wire shape. * + * Reads a raw map because the bridges do not all hand over `Map` + * specifically, and because an unchecked cast that failed would be indistinguishable + * from a reason that was never there. + * * An unrecognized code degrades to [ErrorCode.UNKNOWN] rather than throwing, so a * wrapper built against an older SDK survives a newer producer emitting a code it has - * never heard of. The original text is preserved on [message] either way. + * never heard of. [message], [backendCode] and [source] are preserved either way, which + * is what keeps a degraded reason diagnosable. */ - fun fromMap(map: Map): Detail = - Detail( - code = codeOf(map[KEY_CODE] as? String), + fun fromMap(map: Map<*, *>): Detail { + val code = codeOf(map[KEY_CODE] as? String) + return Detail( + code = code, backendCode = (map[KEY_BACKEND_CODE] as? Number)?.toInt(), message = map[KEY_MESSAGE] as? String, + source = sourceOf(map[KEY_SOURCE] as? String) ?: code.source, ) + } private fun codeOf(name: String?): ErrorCode = ErrorCode.entries.firstOrNull { it.name == name } ?: ErrorCode.UNKNOWN + + private fun sourceOf(name: String?): ErrorSource? = ErrorSource.entries.firstOrNull { it.name == name } } } @@ -149,12 +171,26 @@ class OneSignalError internal constructor( ): OneSignalError = OneSignalError(listOf(Detail(code, backendCode, message)), cause) /** - * Rebuilds an error from its wire shape. A payload carrying no recognizable reason still - * yields a usable error rather than an empty list, so [first] is always safe. + * Rebuilds an error from its wire shape. + * + * Takes the raw value rather than a typed list because the bridges do not all hand over a + * [List] — org.json's array is not one. Anything a producer put under `error` is a failure + * being reported, so an unreadable shape becomes a reason carrying its own text rather than + * being dropped, which would silently turn the failure into a success. + * + * A payload carrying no recognizable reason still yields a usable error rather than an + * empty list, so [first] is always safe. */ - fun fromList(reasons: List>): OneSignalError = - OneSignalError( - reasons.map { Detail.fromMap(it) }.takeIf { it.isNotEmpty() } ?: listOf(Detail(ErrorCode.UNKNOWN)), - ) + fun fromWire(raw: Any?): OneSignalError { + val reasons = + when (raw) { + is List<*> -> raw.map { reasonOf(it) } + else -> listOf(reasonOf(raw)) + } + return OneSignalError(reasons.ifEmpty { listOf(Detail(ErrorCode.UNKNOWN)) }) + } + + private fun reasonOf(raw: Any?): Detail = + if (raw is Map<*, *>) Detail.fromMap(raw) else Detail(ErrorCode.UNKNOWN, message = raw?.toString()) } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt index deed236e5..327b0904e 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt @@ -32,6 +32,13 @@ class OneSignalResult internal constructor( /** The failure detail on failure, `null` on success. */ val error: OneSignalError?, ) { + init { + // [isSuccess] reads error while [getOrThrow] reads data, so an envelope carrying neither or + // both leaves the two disagreeing with nothing to arbitrate. Mirrors the same guard in + // OneSignalError, and means no caller of this constructor can build a result that lies. + require((data == null) != (error == null)) { "OneSignalResult carries exactly one of data or error." } + } + /** `true` when the call completed successfully. Equivalent to `error == null`. */ val isSuccess: Boolean get() = error == null @@ -43,7 +50,7 @@ class OneSignalResult internal constructor( * Returns the payload, or throws [OneSignalException] when the call failed. Use this only where * a failure genuinely cannot be handled locally. */ - fun getOrThrow(): T = data ?: throw OneSignalException(error ?: unexpectedMissingError()) + fun getOrThrow(): T = data ?: throw OneSignalException(checkNotNull(error)) /** Projects the envelope onto the cross-SDK wire shape consumed by the wrapper bridges. */ fun toMap(): Map = @@ -55,8 +62,6 @@ class OneSignalResult internal constructor( override fun toString(): String = if (isSuccess) "OneSignalResult(success, data=$data)" else "OneSignalResult(failure, error=$error)" - private fun unexpectedMissingError() = OneSignalError.of(ErrorCode.UNKNOWN, "Result carried neither data nor error.") - internal companion object { // Private because `const val` in an internal companion still compiles to a public static // field, which would leak the wire keys into the customer-facing API surface. @@ -82,15 +87,19 @@ class OneSignalResult internal constructor( * truth, which keeps a malformed producer from yielding a result that claims success while * carrying an error. Unrecognized keys are ignored so a newer producer can add fields * without breaking an older consumer. + * + * `error` is tested for presence, never for shape. A cast doing double duty as the + * predicate would read an error the parser could not type as no error at all, and report + * the failure as an empty success. */ @Suppress("UNCHECKED_CAST") fun fromMap( map: Map, dataParser: (Map) -> T, ): OneSignalResult { - val reasons = map[KEY_ERROR] as? List> + val reasons = map[KEY_ERROR] if (reasons != null) { - return failure(OneSignalError.fromList(reasons)) + return failure(OneSignalError.fromWire(reasons)) } val dataMap = map[KEY_DATA] as? Map ?: emptyMap() diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt index 3725a7324..c19d58bc5 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt @@ -1,6 +1,7 @@ package com.onesignal import io.kotest.assertions.throwables.shouldThrow +import io.kotest.assertions.withClue import io.kotest.core.spec.style.FunSpec import io.kotest.matchers.booleans.shouldBeFalse import io.kotest.matchers.booleans.shouldBeTrue @@ -8,6 +9,7 @@ import io.kotest.matchers.nulls.shouldBeNull import io.kotest.matchers.nulls.shouldNotBeNull import io.kotest.matchers.shouldBe import io.kotest.matchers.types.shouldBeInstanceOf +import org.json.JSONArray class OneSignalResultTests : FunSpec({ @@ -37,10 +39,10 @@ class OneSignalResultTests : FunSpec({ val client = OneSignalResult.failure(ErrorCode.STORAGE_LOCKED, "device locked") val backend = OneSignalResult.failure(ErrorCode.BACKEND_ERROR, "Invalid API Key", backendCode = 100) - client.error!!.first.code.source shouldBe ErrorSource.CLIENT + client.error!!.first.source shouldBe ErrorSource.CLIENT client.error!!.first.backendCode.shouldBeNull() - backend.error!!.first.code.source shouldBe ErrorSource.BACKEND + backend.error!!.first.source shouldBe ErrorSource.BACKEND backend.error!!.first.backendCode shouldBe 100 } @@ -244,4 +246,136 @@ class OneSignalResultTests : FunSpec({ restored.data.shouldBeNull() restored.error!!.first.code shouldBe ErrorCode.STORAGE_LOCKED } + + // The version above only holds when the error happens to be a well-typed List. The rule is that + // a present error wins whatever shape it arrives in, so the awkward shapes are checked too. + test("error presence wins over a contradictory success flag whatever shape the error arrives in") { + val shapes = + listOf( + JSONArray("""[{"code":"STORAGE_LOCKED"}]"""), + mapOf("code" to "STORAGE_LOCKED"), + "STORAGE_LOCKED", + listOf("STORAGE_LOCKED"), + ) + + shapes.forEach { shape -> + val restored = + OneSignalResult.fromMap( + mapOf( + "success" to true, + "data" to mapOf("onesignalId" to "os-1", "externalId" to "ext-1"), + "error" to shape, + ), + LoginData::fromMap, + ) + + withClue("error carried as ${shape.javaClass.simpleName}") { + restored.isSuccess.shouldBeFalse() + restored.data.shouldBeNull() + restored.error.shouldNotBeNull() + } + } + } + + // org.json is what a bridge naturally parses with, and JSONArray is not a java.util.List. Reading + // the error with a cast that doubles as the predicate turned that into a blank success. + test("an error arriving as a JSONArray still reports a failure") { + val fromBridge = + mapOf( + "success" to false, + "data" to null, + "error" to JSONArray("""[{"code":"STORAGE_LOCKED","source":"CLIENT","message":"device locked"}]"""), + ) + + val restored = OneSignalResult.fromMap(fromBridge, LoginData::fromMap) + + restored.isSuccess.shouldBeFalse() + restored.data.shouldBeNull() + restored.error.shouldNotBeNull() + } + + test("an error list holding a reason that is not a map reports a failure instead of throwing") { + val restored = + OneSignalResult.fromMap( + mapOf("success" to false, "data" to null, "error" to listOf("something broke")), + LoginData::fromMap, + ) + + restored.isSuccess.shouldBeFalse() + restored.error!!.first.code shouldBe ErrorCode.UNKNOWN + restored.error!!.first.message shouldBe "something broke" + } + + test("a reason list mixing a readable reason with an unreadable one keeps both") { + val restored = + OneSignalResult.fromMap( + mapOf("error" to listOf(mapOf("code" to "STORAGE_LOCKED"), 42)), + LoginData::fromMap, + ) + + restored.error!!.error.size shouldBe 2 + restored.error!!.error[0].code shouldBe ErrorCode.STORAGE_LOCKED + restored.error!!.error[1].code shouldBe ErrorCode.UNKNOWN + restored.error!!.error[1].message shouldBe "42" + } + + // isSuccess reads error while getOrThrow reads data, so an envelope carrying neither or both + // makes the two disagree. The constructor is the only place that can rule it out. + test("a result cannot be built carrying neither data nor error") { + shouldThrow { OneSignalResult(null, null) } + } + + test("a result cannot be built carrying both data and error") { + shouldThrow { + OneSignalResult(LoginData("os-1", "ext-1"), OneSignalError.of(ErrorCode.UNKNOWN)) + } + } + + // The copy stops a caller emptying the list that was passed in, but Java can still clear the + // copy itself, and first is documented as always safe to read. + test("the reason list handed to callers cannot be emptied") { + val restored = + OneSignalResult.fromMap( + mapOf("error" to listOf(mapOf("code" to "BACKEND_ERROR"), mapOf("code" to "STORAGE_LOCKED"))), + LoginData::fromMap, + ) + + @Suppress("UNCHECKED_CAST") + shouldThrow { (restored.error!!.error as MutableList).clear() } + + restored.error!!.first.code shouldBe ErrorCode.BACKEND_ERROR + } + + // Attribution is the point of source. Re-deriving it from a code that has already degraded to + // UNKNOWN hands back a client-attributed error carrying a backend code. + test("an unrecognized code keeps the source the producer sent") { + val fromNewerProducer = + mapOf( + "success" to false, + "data" to null, + "error" to + listOf( + mapOf("code" to "RATE_LIMITED", "source" to "BACKEND", "backendCode" to 429, "message" to "slow down"), + ), + ) + + val restored = OneSignalResult.fromMap(fromNewerProducer, LoginData::fromMap) + + restored.error!!.first.code shouldBe ErrorCode.UNKNOWN + restored.error!!.first.backendCode shouldBe 429 + restored.error!!.first.source shouldBe ErrorSource.BACKEND + // Also checked through the wire projection, since toMap and fromMap have to stay symmetric. + restored.error!!.toList().first()["source"] shouldBe "BACKEND" + } + + test("a reason with no source on the wire falls back to the source its code implies") { + val restored = + OneSignalResult.fromMap( + mapOf("error" to listOf(mapOf("code" to "BACKEND_ERROR", "backendCode" to 100))), + LoginData::fromMap, + ) + + restored.error!!.first.source shouldBe ErrorSource.BACKEND + restored.error!!.toList().first()["source"] shouldBe "BACKEND" + } }) From be55a03dc87a60af2b7d0b134c92ca1de11b16dc Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Tue, 11 Aug 2026 12:18:09 -0500 Subject: [PATCH 3/4] fix: report an unreadable result payload instead of substituting a blank one The data half of the envelope still had the cast-as-predicate shape just removed from error: `map["data"] as? Map ?: emptyMap()` read a payload the parser could not type as no payload at all. A JSONObject, which is what a bridge naturally parses with and is not a kotlin Map, produced LoginData("", "") under a result still reporting success, and the either/or require could not catch it because the parser always returns a payload. Presence and shape are now separate. Absent or null data still means a payload with no fields, which is what the empty payload types serialize to. Data present in a shape the parser cannot read becomes a failure carrying an UNKNOWN reason naming the offending type. Reporting a failure contradicts a producer that was signalling success, and is still the lesser harm. The envelope exists so that isSuccess tells a caller whether data can be trusted, and a fabricated payload is indistinguishable at the call site from a real one. A caller handed a failure retries or reports it; a caller handed a blank payload logs someone in as nobody. The message names the type only, since the payload is identity data and the message gets logged. The payload parsers now take a raw map, matching Detail.fromMap, which removes the last unchecked cast here. All four are internal, so the public API is unchanged. Field-level validation inside the payloads is deliberately left alone. Making LoginData reject a missing onesignalId requires the parser contract to become fallible, which is a design change rather than a bug fix. --- .../java/com/onesignal/OneSignalResult.kt | 31 +++++++--- .../java/com/onesignal/OneSignalResultData.kt | 8 +-- .../com/onesignal/OneSignalResultTests.kt | 61 +++++++++++++++++++ 3 files changed, 89 insertions(+), 11 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt index 327b0904e..a6f7d5aee 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt @@ -88,23 +88,40 @@ class OneSignalResult internal constructor( * carrying an error. Unrecognized keys are ignored so a newer producer can add fields * without breaking an older consumer. * - * `error` is tested for presence, never for shape. A cast doing double duty as the - * predicate would read an error the parser could not type as no error at all, and report - * the failure as an empty success. + * Neither `error` nor `data` is read with a cast that doubles as its own presence check. + * Such a cast reports a value the parser could not type as a value that was never sent, + * which turns a failure into an empty success and an unreadable payload into a blank one. + * + * A payload that is present but unreadable is reported as a failure. That contradicts the + * producer, which was reporting success, and it is still the lesser harm: the envelope's + * whole purpose is that [isSuccess] tells a caller whether [data] can be trusted, and a + * fabricated payload is indistinguishable at the call site from one the producer really + * sent. A caller handed a failure retries or reports it; a caller handed a blank payload + * logs someone in as nobody. */ - @Suppress("UNCHECKED_CAST") fun fromMap( map: Map, - dataParser: (Map) -> T, + dataParser: (Map<*, *>) -> T, ): OneSignalResult { val reasons = map[KEY_ERROR] if (reasons != null) { return failure(OneSignalError.fromWire(reasons)) } - val dataMap = map[KEY_DATA] as? Map ?: emptyMap() - return success(dataParser(dataMap)) + return when (val payload = map[KEY_DATA]) { + // Absent or null is a payload with no fields, which is exactly what the empty + // payload types serialize to. Only a payload sent in a shape that cannot be read + // is a failure. + null -> success(dataParser(emptyMap())) + is Map<*, *> -> success(dataParser(payload)) + else -> failure(ErrorCode.UNKNOWN, unreadablePayload(payload)) + } } + + // The type is enough to diagnose the producer. The payload itself is identity data, and an + // error message is somewhere it would be logged. + private fun unreadablePayload(payload: Any): String = + "Result data could not be read: expected a map but received ${payload::class.simpleName}." } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt index 73a9b5032..00f84bdee 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt @@ -44,7 +44,7 @@ class LoginData internal constructor( private const val KEY_ONESIGNAL_ID = "onesignalId" private const val KEY_EXTERNAL_ID = "externalId" - fun fromMap(map: Map): LoginData = + fun fromMap(map: Map<*, *>): LoginData = LoginData( onesignalId = map[KEY_ONESIGNAL_ID] as? String ?: "", externalId = map[KEY_EXTERNAL_ID] as? String ?: "", @@ -60,7 +60,7 @@ class LogoutData internal constructor() : OneSignalResultData { internal companion object { fun fromMap( - @Suppress("UNUSED_PARAMETER") map: Map, + @Suppress("UNUSED_PARAMETER") map: Map<*, *>, ): LogoutData = LogoutData() } } @@ -73,7 +73,7 @@ class UpdateUserJwtData internal constructor() : OneSignalResultData { internal companion object { fun fromMap( - @Suppress("UNUSED_PARAMETER") map: Map, + @Suppress("UNUSED_PARAMETER") map: Map<*, *>, ): UpdateUserJwtData = UpdateUserJwtData() } } @@ -86,7 +86,7 @@ class InitData internal constructor() : OneSignalResultData { internal companion object { fun fromMap( - @Suppress("UNUSED_PARAMETER") map: Map, + @Suppress("UNUSED_PARAMETER") map: Map<*, *>, ): InitData = InitData() } } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt index c19d58bc5..cb3d7b1b0 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt @@ -8,8 +8,11 @@ import io.kotest.matchers.booleans.shouldBeTrue import io.kotest.matchers.nulls.shouldBeNull import io.kotest.matchers.nulls.shouldNotBeNull import io.kotest.matchers.shouldBe +import io.kotest.matchers.string.shouldContain +import io.kotest.matchers.string.shouldNotContain import io.kotest.matchers.types.shouldBeInstanceOf import org.json.JSONArray +import org.json.JSONObject class OneSignalResultTests : FunSpec({ @@ -368,6 +371,64 @@ class OneSignalResultTests : FunSpec({ restored.error!!.toList().first()["source"] shouldBe "BACKEND" } + // The mirror of the error case: data was read with a cast that doubled as the presence check, + // so a payload the parser could not type read as no payload at all and was replaced with a + // blank one, under a result still claiming success. + test("a payload arriving as a JSONObject reports a failure instead of a blank success") { + val fromBridge = + mapOf( + "success" to true, + "data" to JSONObject("""{"onesignalId":"os-1","externalId":"ext-1"}"""), + "error" to null, + ) + + val restored = OneSignalResult.fromMap(fromBridge, LoginData::fromMap) + + restored.isSuccess.shouldBeFalse() + restored.data.shouldBeNull() + restored.error!!.first.code shouldBe ErrorCode.UNKNOWN + } + + test("a payload that is not a map at all reports a failure") { + val restored = + OneSignalResult.fromMap( + mapOf("success" to true, "data" to "os-1", "error" to null), + LoginData::fromMap, + ) + + restored.isSuccess.shouldBeFalse() + restored.error!!.first.code shouldBe ErrorCode.UNKNOWN + } + + // Naming the type is enough to diagnose the bridge; the payload itself is identity data and has + // no business being copied into an error message that will be logged. + test("an unreadable payload is named by its type without its contents being echoed") { + val restored = + OneSignalResult.fromMap( + mapOf("data" to JSONObject("""{"onesignalId":"os-1","externalId":"user@example.com"}""")), + LoginData::fromMap, + ) + + val message = restored.error!!.first.message!! + message shouldContain "JSONObject" + message shouldNotContain "user@example.com" + message shouldNotContain "os-1" + } + + // Regression guard, not evidence: this passes before the fix too. The empty payload types + // serialize to no fields, so an absent or null data has to keep meaning an empty payload + // rather than being swept up as unreadable. + test("an absent or null payload still yields an empty payload rather than a failure") { + listOf(mapOf("success" to true), mapOf("success" to true, "data" to null)).forEach { envelope -> + withClue("envelope $envelope") { + val restored = OneSignalResult.fromMap(envelope, InitData::fromMap) + + restored.isSuccess.shouldBeTrue() + restored.data.shouldNotBeNull() + } + } + } + test("a reason with no source on the wire falls back to the source its code implies") { val restored = OneSignalResult.fromMap( From f3dd107c392279de9f85574c143c8b7c21cf98c9 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Tue, 11 Aug 2026 14:17:30 -0500 Subject: [PATCH 4/4] fix: seal payloads, close JVM constructors, and parse org.json wire shapes Closes the remaining review gaps: OneSignalResultData is sealed, envelope constructors are private so Java cannot fabricate invalid results, and JSONArray/JSONObject bridge payloads are converted instead of degraded. Co-authored-by: Cursor --- OneSignalSDK/onesignal/core/api/core.api | 3 + .../main/java/com/onesignal/OneSignalError.kt | 46 ++++++++++--- .../java/com/onesignal/OneSignalResult.kt | 24 +++++-- .../java/com/onesignal/OneSignalResultData.kt | 7 +- .../com/onesignal/OneSignalResultTests.kt | 68 +++++++++++++------ 5 files changed, 108 insertions(+), 40 deletions(-) diff --git a/OneSignalSDK/onesignal/core/api/core.api b/OneSignalSDK/onesignal/core/api/core.api index 0e51dd3dd..6b485d0c9 100644 --- a/OneSignalSDK/onesignal/core/api/core.api +++ b/OneSignalSDK/onesignal/core/api/core.api @@ -147,6 +147,7 @@ public final class com/onesignal/OneSignal { } public final class com/onesignal/OneSignalError { + public synthetic fun (Ljava/util/List;Ljava/lang/Throwable;Lkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun getCause ()Ljava/lang/Throwable; public final fun getError ()Ljava/util/List; public final fun getFirst ()Lcom/onesignal/OneSignalError$Detail; @@ -155,6 +156,7 @@ public final class com/onesignal/OneSignalError { } public final class com/onesignal/OneSignalError$Detail { + public synthetic fun (Lcom/onesignal/ErrorCode;Ljava/lang/Integer;Ljava/lang/String;Lcom/onesignal/ErrorSource;Lkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun getBackendCode ()Ljava/lang/Integer; public final fun getCode ()Lcom/onesignal/ErrorCode; public final fun getMessage ()Ljava/lang/String; @@ -168,6 +170,7 @@ public final class com/onesignal/OneSignalException : java/lang/Exception { } public final class com/onesignal/OneSignalResult { + public synthetic fun (Lcom/onesignal/OneSignalResultData;Lcom/onesignal/OneSignalError;Lkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun getData ()Lcom/onesignal/OneSignalResultData; public final fun getError ()Lcom/onesignal/OneSignalError; public final fun getOrNull ()Lcom/onesignal/OneSignalResultData; diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt index 2a1ec0da0..d00164b67 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalError.kt @@ -1,5 +1,9 @@ package com.onesignal +import com.onesignal.common.toList +import com.onesignal.common.toMap +import org.json.JSONArray +import org.json.JSONObject import java.util.Collections /** Whether the SDK produced a failure locally or OneSignal's backend returned it. */ @@ -48,8 +52,12 @@ enum class ErrorCode(val source: ErrorSource) { * { "success": false, "data": null, * "error": [ { "code": "STORAGE_LOCKED", "source": "CLIENT", "backendCode": null, "message": "..." } ] } * ``` + * + * The constructor is private on purpose. An `internal` constructor still emits as JVM-public, so + * Java outside this module could build an error with no reasons and leave [first] throwing; private + * closes that hole. Callers construct through [of] or [fromWire]. */ -class OneSignalError internal constructor( +class OneSignalError private constructor( error: List, /** * The throwable behind the failure, when there was one. @@ -59,7 +67,7 @@ class OneSignalError internal constructor( * Java callers do not lose the stack when the suspend APIs report a failure instead of * throwing it. */ - val cause: Throwable? = null, + val cause: Throwable?, ) { /** * Why the call failed. Never empty. @@ -83,7 +91,7 @@ class OneSignalError internal constructor( * Nested rather than top-level so the name cannot collide with `kotlin.Error`, which is * auto-imported everywhere, or shadow `java.lang.Error` in a Java file that imports it. */ - class Detail internal constructor( + class Detail private constructor( /** A stable code, safe to branch on. Never localized. */ val code: ErrorCode, /** @@ -92,9 +100,9 @@ class OneSignalError internal constructor( * Left as a raw number on purpose: the backend adds codes on its own schedule, and an SDK * release must not be the thing that unblocks recognizing one. */ - val backendCode: Int? = null, + val backendCode: Int?, /** A human-readable description intended for logs and diagnostics, not for end users. */ - val message: String? = null, + val message: String?, /** * Who the failure came from. * @@ -103,7 +111,7 @@ class OneSignalError internal constructor( * backend failure as a client one. Defaults to the source [code] implies, which is right * for everything the SDK raises locally. */ - val source: ErrorSource = code.source, + val source: ErrorSource, ) { /** Projects this reason onto the cross-SDK wire shape consumed by the wrapper bridges. */ fun toMap(): Map = @@ -146,6 +154,13 @@ class OneSignalError internal constructor( ) } + fun of( + code: ErrorCode, + backendCode: Int? = null, + message: String? = null, + source: ErrorSource = code.source, + ): Detail = Detail(code, backendCode, message, source) + private fun codeOf(name: String?): ErrorCode = ErrorCode.entries.firstOrNull { it.name == name } ?: ErrorCode.UNKNOWN private fun sourceOf(name: String?): ErrorSource? = ErrorSource.entries.firstOrNull { it.name == name } @@ -168,7 +183,13 @@ class OneSignalError internal constructor( message: String? = null, backendCode: Int? = null, cause: Throwable? = null, - ): OneSignalError = OneSignalError(listOf(Detail(code, backendCode, message)), cause) + ): OneSignalError = OneSignalError(listOf(Detail.of(code, backendCode, message)), cause) + + /** Builds a multi-reason error. [reasons] must not be empty. */ + fun of( + reasons: List, + cause: Throwable? = null, + ): OneSignalError = OneSignalError(reasons, cause) /** * Rebuilds an error from its wire shape. @@ -185,12 +206,19 @@ class OneSignalError internal constructor( val reasons = when (raw) { is List<*> -> raw.map { reasonOf(it) } + // org.json is what a bridge naturally parses with, and JSONArray is not a + // java.util.List. Convert rather than treating the whole array as one reason. + is JSONArray -> raw.toList().orEmpty().map { reasonOf(it) } else -> listOf(reasonOf(raw)) } - return OneSignalError(reasons.ifEmpty { listOf(Detail(ErrorCode.UNKNOWN)) }) + return OneSignalError(reasons.ifEmpty { listOf(Detail.of(ErrorCode.UNKNOWN)) }, cause = null) } private fun reasonOf(raw: Any?): Detail = - if (raw is Map<*, *>) Detail.fromMap(raw) else Detail(ErrorCode.UNKNOWN, message = raw?.toString()) + when (raw) { + is Map<*, *> -> Detail.fromMap(raw) + is JSONObject -> Detail.fromMap(raw.toMap()) + else -> Detail.of(ErrorCode.UNKNOWN, message = raw?.toString()) + } } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt index a6f7d5aee..64280b64f 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResult.kt @@ -1,5 +1,8 @@ package com.onesignal +import com.onesignal.common.toMap +import org.json.JSONObject + /** * The outcome of an asynchronous OneSignal call: either a [data] payload or an [error], never both * and never neither. @@ -15,6 +18,10 @@ package com.onesignal * The presence of [error] is what defines the outcome; [isSuccess] and the wire-level `success` * flag are both derived from it, so the two can never disagree. * + * The constructor is private on purpose. An `internal` constructor still emits as JVM-public, so + * Java outside this module could build an envelope that violates the either/or invariant; private + * closes that hole. Callers construct through [success], [failure], or [fromMap]. + * * From Kotlin: * ```kotlin * val result = OneSignal.login("user-123") @@ -26,7 +33,7 @@ package com.onesignal * if (result.isSuccess()) { result.getData(); } else { result.getError(); } * ``` */ -class OneSignalResult internal constructor( +class OneSignalResult private constructor( /** The payload on success, `null` on failure. */ val data: T?, /** The failure detail on failure, `null` on success. */ @@ -92,12 +99,12 @@ class OneSignalResult internal constructor( * Such a cast reports a value the parser could not type as a value that was never sent, * which turns a failure into an empty success and an unreadable payload into a blank one. * - * A payload that is present but unreadable is reported as a failure. That contradicts the - * producer, which was reporting success, and it is still the lesser harm: the envelope's - * whole purpose is that [isSuccess] tells a caller whether [data] can be trusted, and a - * fabricated payload is indistinguishable at the call site from one the producer really - * sent. A caller handed a failure retries or reports it; a caller handed a blank payload - * logs someone in as nobody. + * A payload that is present but unreadable (neither a [Map] nor a [JSONObject]) is reported + * as a failure. That contradicts the producer, which was reporting success, and it is still + * the lesser harm: the envelope's whole purpose is that [isSuccess] tells a caller whether + * [data] can be trusted, and a fabricated payload is indistinguishable at the call site from + * one the producer really sent. A caller handed a failure retries or reports it; a caller + * handed a blank payload logs someone in as nobody. */ fun fromMap( map: Map, @@ -114,6 +121,9 @@ class OneSignalResult internal constructor( // is a failure. null -> success(dataParser(emptyMap())) is Map<*, *> -> success(dataParser(payload)) + // org.json is what a bridge naturally parses with. JSONObject is not a kotlin Map, + // but it is a map on the wire — convert rather than report a blank failure. + is JSONObject -> success(dataParser(payload.toMap())) else -> failure(ErrorCode.UNKNOWN, unreadablePayload(payload)) } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt index 00f84bdee..052ddce1b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignalResultData.kt @@ -3,10 +3,11 @@ package com.onesignal /** * Implemented by every payload that can travel inside a [OneSignalResult]. * - * The contract exists so the envelope can be projected onto the wire shape without knowing which - * payload it is carrying. It is not intended to be implemented outside the SDK. + * Sealed so a new member is never a silent source-compatible break for external implementors — this + * contract exists so the envelope can project onto the wire shape without knowing which payload it + * is carrying, and is not intended to be implemented outside the SDK. */ -interface OneSignalResultData { +sealed interface OneSignalResultData { /** Projects this payload onto the cross-SDK wire shape consumed by the wrapper bridges. */ fun toMap(): Map } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt index cb3d7b1b0..393d65f21 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/OneSignalResultTests.kt @@ -51,10 +51,10 @@ class OneSignalResultTests : FunSpec({ test("an error can carry several reasons at once") { val error = - OneSignalError( + OneSignalError.of( listOf( - OneSignalError.Detail(ErrorCode.BACKEND_ERROR, 100, "Invalid API Key"), - OneSignalError.Detail(ErrorCode.BACKEND_ERROR, 144, "Invalid external ID"), + OneSignalError.Detail.of(ErrorCode.BACKEND_ERROR, 100, "Invalid API Key"), + OneSignalError.Detail.of(ErrorCode.BACKEND_ERROR, 144, "Invalid external ID"), ), ) @@ -63,10 +63,10 @@ class OneSignalResultTests : FunSpec({ error.toList().map { it["backendCode"] } shouldBe listOf(100, 144) } - // first is documented as always safe to read, so the constructor has to refuse the one input - // that would make it throw. + // first is documented as always safe to read, so the factories have to refuse the one input + // that would make it throw. The constructor is private; this is the supported construction path. test("an error cannot be built with no reasons") { - shouldThrow { OneSignalError(emptyList()) } + shouldThrow { OneSignalError.of(emptyList()) } } test("a reason with no message keeps the exception text free of a bare null") { @@ -281,7 +281,8 @@ class OneSignalResultTests : FunSpec({ } // org.json is what a bridge naturally parses with, and JSONArray is not a java.util.List. Reading - // the error with a cast that doubles as the predicate turned that into a blank success. + // the error with a cast that doubled as the predicate turned that into a blank success; treating + // the whole array as one opaque reason would also lose the codes. Convert and parse each entry. test("an error arriving as a JSONArray still reports a failure") { val fromBridge = mapOf( @@ -294,7 +295,9 @@ class OneSignalResultTests : FunSpec({ restored.isSuccess.shouldBeFalse() restored.data.shouldBeNull() - restored.error.shouldNotBeNull() + restored.error!!.first.code shouldBe ErrorCode.STORAGE_LOCKED + restored.error!!.first.message shouldBe "device locked" + restored.error!!.first.source shouldBe ErrorSource.CLIENT } test("an error list holding a reason that is not a map reports a failure instead of throwing") { @@ -323,15 +326,37 @@ class OneSignalResultTests : FunSpec({ } // isSuccess reads error while getOrThrow reads data, so an envelope carrying neither or both - // makes the two disagree. The constructor is the only place that can rule it out. + // makes the two disagree. The constructor is private and the factories only emit one side; the + // require stays as defense for a future factory bug and is reached here via reflection so the + // JVM-public accidental entry point Fadi flagged cannot reopen quietly. test("a result cannot be built carrying neither data nor error") { - shouldThrow { OneSignalResult(null, null) } + val ctor = + OneSignalResult::class.java.getDeclaredConstructor( + OneSignalResultData::class.java, + OneSignalError::class.java, + ) + ctor.isAccessible = true + + val thrown = + shouldThrow { + ctor.newInstance(null, null) + } + thrown.cause.shouldBeInstanceOf() } test("a result cannot be built carrying both data and error") { - shouldThrow { - OneSignalResult(LoginData("os-1", "ext-1"), OneSignalError.of(ErrorCode.UNKNOWN)) - } + val ctor = + OneSignalResult::class.java.getDeclaredConstructor( + OneSignalResultData::class.java, + OneSignalError::class.java, + ) + ctor.isAccessible = true + + val thrown = + shouldThrow { + ctor.newInstance(LoginData("os-1", "ext-1"), OneSignalError.of(ErrorCode.UNKNOWN)) + } + thrown.cause.shouldBeInstanceOf() } // The copy stops a caller emptying the list that was passed in, but Java can still clear the @@ -373,8 +398,9 @@ class OneSignalResultTests : FunSpec({ // The mirror of the error case: data was read with a cast that doubled as the presence check, // so a payload the parser could not type read as no payload at all and was replaced with a - // blank one, under a result still claiming success. - test("a payload arriving as a JSONObject reports a failure instead of a blank success") { + // blank one, under a result still claiming success. JSONObject is the bridge's natural map + // shape, so convert it rather than reporting a failure that contradicts a successful producer. + test("a payload arriving as a JSONObject is read as a success") { val fromBridge = mapOf( "success" to true, @@ -384,9 +410,9 @@ class OneSignalResultTests : FunSpec({ val restored = OneSignalResult.fromMap(fromBridge, LoginData::fromMap) - restored.isSuccess.shouldBeFalse() - restored.data.shouldBeNull() - restored.error!!.first.code shouldBe ErrorCode.UNKNOWN + restored.isSuccess.shouldBeTrue() + restored.data!!.onesignalId shouldBe "os-1" + restored.data!!.externalId shouldBe "ext-1" } test("a payload that is not a map at all reports a failure") { @@ -405,14 +431,14 @@ class OneSignalResultTests : FunSpec({ test("an unreadable payload is named by its type without its contents being echoed") { val restored = OneSignalResult.fromMap( - mapOf("data" to JSONObject("""{"onesignalId":"os-1","externalId":"user@example.com"}""")), + mapOf("data" to listOf("os-1", "ext-1")), LoginData::fromMap, ) val message = restored.error!!.first.message!! - message shouldContain "JSONObject" - message shouldNotContain "user@example.com" + message shouldContain "ArrayList" message shouldNotContain "os-1" + message shouldNotContain "ext-1" } // Regression guard, not evidence: this passes before the fix too. The empty payload types