diff --git a/OneSignal-KMP-SDK b/OneSignal-KMP-SDK index ee61d4673..89e5f2776 160000 --- a/OneSignal-KMP-SDK +++ b/OneSignal-KMP-SDK @@ -1 +1 @@ -Subproject commit ee61d4673265876e5cab6328b7a0745f79b7a5aa +Subproject commit 89e5f27766225f42f1c45d5c8918eb11ee5e5bc0 diff --git a/OneSignalSDK/detekt/detekt-baseline-core.xml b/OneSignalSDK/detekt/detekt-baseline-core.xml index a46e9d40e..3b38acb97 100644 --- a/OneSignalSDK/detekt/detekt-baseline-core.xml +++ b/OneSignalSDK/detekt/detekt-baseline-core.xml @@ -537,7 +537,7 @@ UndocumentedPublicFunction:IMigrationRecovery.kt$IMigrationRecovery$fun isInBadState(): Boolean UndocumentedPublicFunction:IMigrationRecovery.kt$IMigrationRecovery$fun recover() UndocumentedPublicFunction:IMigrationRecovery.kt$IMigrationRecovery$fun recoveryMessage(): String - UndocumentedPublicFunction:IOneSignal.kt$IOneSignal$fun login(externalId: String) + UndocumentedPublicFunction:IOneSignal.kt$IOneSignal$fun login(externalId: String?) UndocumentedPublicFunction:IOperationRepo.kt$IOperationRepo$fun forceExecuteOperations() UndocumentedPublicFunction:IOperationRepo.kt$IOperationRepo$suspend fun awaitInitialized() UndocumentedPublicFunction:IOperationRepo.kt$inline fun <reified T : Operation> IOperationRepo.containsInstanceOf(): Boolean diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/IOneSignal.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/IOneSignal.kt index 3bb1ed8ee..265b2bcb3 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/IOneSignal.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/IOneSignal.kt @@ -78,9 +78,9 @@ interface IOneSignal { /** * Initialize the OneSignal SDK. This should be called during startup of the application. * - * @param context The Android context the SDK should use. Required: a null context is logged and init does not start. + * @param context The Android context the SDK should use. Required: a null context is logged and init does not start. The log says the SDK is not initialized. * @param appId Your OneSignal app ID. Always pass it: a null or invalid value falls back to the app ID - * cached by an earlier launch, and with none cached init fails and logs. + * cached by an earlier launch, and with none cached init fails and logs that the SDK is not initialized. * * @return true once initialization has started (or is already running or done), false for a * null [context] or an immediate startup failure. Check [isInitialized] for the outcome. @@ -93,9 +93,9 @@ interface IOneSignal { /** * Initialize the OneSignal SDK, suspend until initialization is completed * - * @param context The Android context the SDK should use. + * @param context The Android context the SDK should use. Required: a null context is logged and init does not start. The log says the SDK is not initialized. */ - suspend fun initWithContext(context: Context): Boolean + suspend fun initWithContext(context: Context?): Boolean /** * Login to OneSignal under the user identified by the [externalId] provided. The act of @@ -113,17 +113,17 @@ interface IOneSignal { * subscriptions from the current user (if there is one) to the newly logged in user. This is * because both Push and IAM are owned by the device. * - * @param externalId The external ID of the user to log in. Must be non-empty: an empty value is ignored and logged. + * @param externalId The external ID of the user to log in. Required: null or empty is ignored and logged. A null value's log says the SDK is not initialized when it is not. * @param jwtBearerToken The optional JWT bearer token generated by your backend to establish * trust for the login operation. Required when identity verification has been enabled. See * [Identity Verification | OneSignal](https://documentation.onesignal.com/docs/identity-verification) */ fun login( - externalId: String, + externalId: String?, jwtBearerToken: String? = null, ) - fun login(externalId: String) = login(externalId, null) + fun login(externalId: String?) = login(externalId, null) /** * Logout the user previously logged in via [login]. The [user] property now references @@ -139,11 +139,11 @@ interface IOneSignal { * [IUserJwtInvalidatedListener.onUserJwtInvalidated] callback). Stores the JWT and * wakes the operation queue so any deferred ops can dispatch with the fresh token. * - * @param externalId The external ID the JWT belongs to. Must be non-empty. + * @param externalId The external ID the JWT belongs to. Required: null or empty is ignored and logged. A null value's log says the SDK is not initialized when it is not. * @param token The new JWT bearer token issued by your backend. Must be non-empty; empty values are ignored and logged. */ fun updateUserJwt( - externalId: String, + externalId: String?, token: String, ) @@ -166,9 +166,9 @@ interface IOneSignal { /** * Initialize the OneSignal SDK, suspend until initialization is completed * - * @param context The Android context the SDK should use. Required: a null context is logged and init does not start. + * @param context The Android context the SDK should use. Required: a null context is logged and init does not start. The log says the SDK is not initialized. * @param appId Your OneSignal app ID. Always pass it: a null or invalid value falls back to the app ID - * cached by an earlier launch, and with none cached init fails and logs. + * cached by an earlier launch, and with none cached init fails and logs that the SDK is not initialized. * * @return true if the SDK could be successfully initialized, false otherwise. */ @@ -243,13 +243,13 @@ interface IOneSignal { * Login a user with external ID and optional JWT token (suspend version). * Handles initialization automatically. * - * @param externalId The external ID of the user to log in. Must be non-empty: an empty value is ignored and logged. + * @param externalId The external ID of the user to log in. Required: null or empty is ignored and logged. A null value's log says the SDK is not initialized when it is not. * @param jwtBearerToken The optional JWT bearer token generated by your backend to establish * trust for the login operation. Required when identity verification has been enabled. * See [Identity Verification | OneSignal](https://documentation.onesignal.com/docs/identity-verification) */ suspend fun loginSuspend( - externalId: String, + externalId: String?, jwtBearerToken: String? = null, ) @@ -273,11 +273,11 @@ interface IOneSignal { * Update the JWT bearer token associated with [externalId] (suspend version). Suspends while * initialization is in progress, then stores the JWT; if the SDK is not initialized it logs instead. * - * @param externalId The external ID the JWT belongs to. Must be non-empty. + * @param externalId The external ID the JWT belongs to. Required: null or empty is ignored and logged. A null value's log says the SDK is not initialized when it is not. * @param token The new JWT bearer token issued by your backend. Must be non-empty; empty values are ignored and logged. */ suspend fun updateUserJwtSuspend( - externalId: String, + externalId: String?, token: String, ) } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignal.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignal.kt index 5e3a7bca0..8a672631e 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignal.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/OneSignal.kt @@ -123,9 +123,9 @@ object OneSignal { /** * Initialize the OneSignal SDK. This should be called during startup of the application. * - * @param context The Android context the SDK should use. Required: a null context is logged and init does not start. + * @param context The Android context the SDK should use. Required: a null context is logged and init does not start. The log says the SDK is not initialized. * @param appId Your OneSignal app ID. Always pass it: a null or invalid value falls back to the app ID - * cached by an earlier launch, and with none cached init fails and logs. Check [isInitialized] for the outcome. + * cached by an earlier launch, and with none cached init fails and logs that the SDK is not initialized. Check [isInitialized] for the outcome. */ @JvmStatic fun initWithContext( @@ -140,9 +140,9 @@ object OneSignal { * This method provides a suspended version that returns a boolean indicating success. * Uses Dispatchers.IO internally to prevent ANRs and optimize for I/O operations. * - * @param context Application context is recommended for SDK operations. Required: a null context is logged and init does not start. + * @param context Application context is recommended for SDK operations. Required: a null context is logged and init does not start. The log says the SDK is not initialized. * @param appId Your OneSignal app ID. Always pass it: a null or invalid value falls back to the app ID - * cached by an earlier launch, and with none cached init fails and logs. + * cached by an earlier launch, and with none cached init fails and logs that the SDK is not initialized. * @return Boolean indicating if initialization was successful. */ @JvmStatic @@ -303,10 +303,10 @@ object OneSignal { * subscriptions from the current user (if there is one) to the newly logged in user. This is * because both Push and IAM are owned by the device. * - * @param externalId The external ID of the user to log in. Must be non-empty: an empty value is ignored and logged. + * @param externalId The external ID of the user to log in. Required: null or empty is ignored and logged. A null value's log says the SDK is not initialized when it is not. */ @JvmStatic - fun login(externalId: String) = oneSignal.login(externalId) + fun login(externalId: String?) = oneSignal.login(externalId) /** * Login to OneSignal under the user identified by the [externalId] provided. The act of @@ -324,14 +324,14 @@ object OneSignal { * subscriptions from the current user (if there is one) to the newly logged in user. This is * because both Push and IAM are owned by the device. * - * @param externalId The external ID of the user to log in. Must be non-empty: an empty value is ignored and logged. + * @param externalId The external ID of the user to log in. Required: null or empty is ignored and logged. A null value's log says the SDK is not initialized when it is not. * @param jwtBearerToken The optional JWT bearer token generated by your backend to establish * trust for the login operation. Required when identity verification has been enabled. See * [Identity Verification | OneSignal](https://documentation.onesignal.com/docs/identity-verification) */ @JvmStatic fun login( - externalId: String, + externalId: String?, jwtBearerToken: String? = null, ) = oneSignal.login(externalId, jwtBearerToken) @@ -350,12 +350,12 @@ object OneSignal { * [IUserJwtInvalidatedListener.onUserJwtInvalidated] callback). Stores the JWT and * wakes the operation queue so any deferred ops can dispatch with the fresh token. * - * @param externalId The external ID the JWT belongs to. Must be non-empty. + * @param externalId The external ID the JWT belongs to. Required: null or empty is ignored and logged. A null value's log says the SDK is not initialized when it is not. * @param token The new JWT bearer token issued by your backend. Must be non-empty; empty values are ignored and logged. */ @JvmStatic fun updateUserJwt( - externalId: String, + externalId: String?, token: String, ) = oneSignal.updateUserJwt(externalId, token) @@ -390,7 +390,7 @@ object OneSignal { * THIS IS AN INTERNAL INTERFACE AND SHOULD NOT BE USED DIRECTLY. */ @JvmStatic - suspend fun initWithContext(context: Context): Boolean { + suspend fun initWithContext(context: Context?): Boolean { return oneSignal.initWithContext(context) } @@ -413,14 +413,14 @@ object OneSignal { * subscriptions from the current user (if there is one) to the newly logged in user. This is * because both Push and IAM are owned by the device. * - * @param externalId The external ID of the user to log in. Must be non-empty: an empty value is ignored and logged. + * @param externalId The external ID of the user to log in. Required: null or empty is ignored and logged. A null value's log says the SDK is not initialized when it is not. * @param jwtBearerToken The optional JWT bearer token generated by your backend to establish * trust for the login operation. Required when identity verification has been enabled. See * [Identity Verification | OneSignal](https://documentation.onesignal.com/docs/identity-verification) */ @JvmStatic suspend fun loginSuspend( - externalId: String, + externalId: String?, jwtBearerToken: String? = null, ) { oneSignal.loginSuspend(externalId, jwtBearerToken) @@ -457,12 +457,12 @@ object OneSignal { * Update the JWT bearer token associated with [externalId] without blocking the calling * thread. Suspend-safe version of [updateUserJwt]. * - * @param externalId The external ID the JWT belongs to. Must be non-empty. + * @param externalId The external ID the JWT belongs to. Required: null or empty is ignored and logged. A null value's log says the SDK is not initialized when it is not. * @param token The new JWT bearer token issued by your backend. Must be non-empty; empty values are ignored and logged. */ @JvmStatic suspend fun updateUserJwtSuspend( - externalId: String, + externalId: String?, token: String, ) { oneSignal.updateUserJwtSuspend(externalId, token) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/InputGuard.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/InputGuard.kt index 593a1eca8..527a9facd 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/InputGuard.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/InputGuard.kt @@ -1,7 +1,6 @@ package com.onesignal.common import com.onesignal.debug.internal.logging.Logging -import com.onesignal.internal.SdkOperation internal fun hasNullByte(value: String?): Boolean = value != null && '\u0000' in value @@ -23,12 +22,6 @@ fun isMissing( return error != null } -internal fun isMissing( - value: String?, - operation: SdkOperation, - parameter: String, -): Boolean = isMissing(value, "$operation: $parameter") - /** * Returns true when any of [values] is missing, per [isMissing]. */ diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/DeferredManagers.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/DeferredManagers.kt index e954ca767..f909d8e12 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/DeferredManagers.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/DeferredManagers.kt @@ -41,6 +41,11 @@ internal open class DeferredManager( private val pending = mutableListOf>() private var replayed = false + // Set for the whole drain. Other threads enqueue instead of touching the real manager, so a remove + // cannot land before the add that was already copied into the batch being applied. + private var replaying = false + private var replayThread: Thread? = null + // Kept after replay: if init then fails, or a replayed listener calls back in, the gate is not ready // but this manager already holds the replayed registrations, so later ones must reach it too. private var replayedTo: T? = null @@ -55,6 +60,7 @@ internal open class DeferredManager( ) = forwardOrQueue(member, call) { pending.size < MAX_PENDING && pending.add(Pending(member, target, call)) } // Before replay the real manager holds nothing, so a removal only has to cancel the queued add. + // Once that add has been copied into the batch in flight, the removal itself is queued behind it. protected fun unregisterOrCancel( member: String, addMember: String, @@ -62,8 +68,14 @@ internal open class DeferredManager( call: (T) -> Unit, ) = forwardOrQueue(member, call) { val index = pending.indexOfLast { it.member == addMember && it.target === target } - if (index >= 0) pending.removeAt(index) - true + if (index >= 0) { + pending.removeAt(index) + true + } else if (replaying) { + pending.size < MAX_PENDING && pending.add(Pending(member, target, call)) + } else { + false + } } // [onQueued] runs under the queue lock, so a getter cache it sets always matches what replays. @@ -85,13 +97,11 @@ internal open class DeferredManager( call: (T) -> Unit, enqueue: () -> Boolean, ) { - source.peek()?.let { - call(it) - return - } val (queued, target) = synchronized(pending) { - if (replayed) false to replayedTo else enqueue() to null + val applying = replaying && Thread.currentThread() === replayThread + val finished = replayed && !replaying + if (applying || finished) false to (replayedTo ?: source.peek()) else enqueue() to null } when { queued -> source.logQueued(member) @@ -100,20 +110,52 @@ internal open class DeferredManager( } } - @Suppress("TooGenericExceptionCaught") open fun replay(real: T) { - val calls = - synchronized(pending) { - replayed = true - replayedTo = real - pending.toList().also { pending.clear() } - } - for (entry in calls) { - try { - entry.call(real) - } catch (e: Exception) { - Logging.error("Replaying '${entry.member}' threw; that queued call is lost", e) + if (!beginReplay(real)) return + try { + while (true) { + val batch = takeBatch() + if (batch.isEmpty()) return + batch.forEach { deliver(it, real) } } + } finally { + abandonReplay() + } + } + + private fun beginReplay(real: T): Boolean = + synchronized(pending) { + if (replayed || replaying) return@synchronized false + replaying = true + replayThread = Thread.currentThread() + replayedTo = real + true + } + + private fun takeBatch(): List> = + synchronized(pending) { + if (pending.isNotEmpty()) return@synchronized pending.toList().also { pending.clear() } + replaying = false + replayThread = null + replayed = true + emptyList() + } + + @Suppress("TooGenericExceptionCaught") + private fun deliver(entry: Pending, real: T) { + try { + entry.call(real) + } catch (e: Exception) { + Logging.error("Replaying '${entry.member}' threw; that queued call is lost", e) + } + } + + private fun abandonReplay() { + synchronized(pending) { + if (!replaying) return@synchronized + replaying = false + replayThread = null + replayed = true } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/InitGate.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/InitGate.kt index 043b9632e..3039f8f26 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/InitGate.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/InitGate.kt @@ -45,7 +45,8 @@ internal class InitGate { private val isOnInitThread: Boolean get() = state == InitState.IN_PROGRESS && initThread.get() === Thread.currentThread() // Every attempt must end: one left IN_PROGRESS blocks every sync accessor forever. - fun runInit(block: () -> Boolean): Boolean { + // [finishIfUnfinished] is false only for the caller's pre-dispatch work, which completes itself. + fun runInit(finishIfUnfinished: Boolean = true, block: () -> Boolean): Boolean { val current = Thread.currentThread() val owned = synchronized(lock) { attempt } initThread.set(current) @@ -56,7 +57,7 @@ internal class InitGate { addFailureCause(e) return false } finally { - failIfUnfinished(owned) + if (finishIfUnfinished) failIfUnfinished(owned) // A retry woken by this attempt's FAILED may already have claimed the slot. initThread.compareAndSet(current, null) } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/NeverCrashKillSwitch.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/NeverCrashKillSwitch.kt index 4478383dd..c822e24de 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/NeverCrashKillSwitch.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/NeverCrashKillSwitch.kt @@ -8,21 +8,22 @@ import com.onesignal.core.internal.config.ConfigModel import com.onesignal.core.internal.preferences.PreferenceOneSignalKeys import com.onesignal.core.internal.preferences.PreferenceStores import com.onesignal.debug.internal.logging.Logging +import com.onesignal.features.FeatureFlag import org.json.JSONArray /** * Chooses the [IOneSignal] implementation once per process, from the feature flags cached by the - * previous run, so a remote change applies on the next cold start. Any failure keeps the new behavior. + * previous run, so a remote change applies on the next cold start. Any failure keeps the legacy behavior. */ internal object NeverCrashKillSwitch { - // Present means off, so installs whose init never succeeds (and never fetch flags) keep the safe default. - const val FLAG_KEY = "sdk_never_crash_disabled" + // Absent means the legacy implementation, so the rollout stays off until Turbine sends the flag. + val FLAG_KEY: String = FeatureFlag.SDK_NEVER_CRASH_ENABLED.key // Local testing only: true forces the legacy implementation, false the new one, null reads the flag. private val localOverride: Boolean? = null @Volatile - var useLegacy: Boolean = false + var useLegacy: Boolean = true private set @Volatile @@ -33,10 +34,10 @@ internal object NeverCrashKillSwitch { @Suppress("TooGenericExceptionCaught") fun evaluate(context: Context) { useLegacy = localOverride ?: try { - isDisabledByCachedFlags(context) + !isEnabledByCachedFlags(context) } catch (t: Throwable) { - Logging.warn("OneSignal: could not read cached feature flags, keeping the default init behavior", t) - false + Logging.warn("OneSignal: could not read cached feature flags, keeping the legacy init behavior", t) + true } evaluated = true } @@ -44,17 +45,17 @@ internal object NeverCrashKillSwitch { fun create(): IOneSignal = when { !evaluated -> { - Logging.debug("OneSignal: kill switch not evaluated before first use, keeping the default init behavior") - OneSignalImpV2() - } - useLegacy -> { - Logging.warn("OneSignal: $FLAG_KEY is on, using the legacy init behavior") + Logging.debug("OneSignal: rollout flag not evaluated before first use, keeping the legacy init behavior") OneSignalImp() } - else -> OneSignalImpV2() + !useLegacy -> { + Logging.warn("OneSignal: $FLAG_KEY is on, using the never-crash init behavior") + OneSignalImpV2() + } + else -> OneSignalImp() } - internal fun isDisabledByCachedFlags(context: Context): Boolean { + internal fun isEnabledByCachedFlags(context: Context): Boolean { // Credential-encrypted prefs throw on access before the user unlocks the device. val keys = if (AndroidUtils.isAndroidUserUnlocked(context)) cachedFeatureKeys(context) else null return keys?.any { it.equals(FLAG_KEY, ignoreCase = true) } == true @@ -70,7 +71,7 @@ internal object NeverCrashKillSwitch { } internal fun resetForTest() { - useLegacy = false + useLegacy = true evaluated = false } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt index b39b136c4..36109912c 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt @@ -399,8 +399,9 @@ internal class OneSignalImp : IOneSignal, /** * Called from internal classes only. Remain suspend until initialization is fully completed. */ - override suspend fun initWithContext(context: Context): Boolean { + override suspend fun initWithContext(context: Context?): Boolean { Logging.log(LogLevel.DEBUG, "initWithContext(context: $context)") + if (context == null) throw nullParameter("initWithContext", "context") return initWithContextSuspend(context, null) } @@ -479,10 +480,11 @@ internal class OneSignalImp : IOneSignal, } override fun login( - externalId: String, + externalId: String?, jwtBearerToken: String?, ) { Logging.log(LogLevel.DEBUG, "Calling deprecated login(externalId: $externalId, jwtBearerToken: ...${jwtBearerToken?.takeLast(8)})") + if (externalId == null) throw nullParameter("login", "externalId") waitForInit(operationName = "login") @@ -504,10 +506,11 @@ internal class OneSignalImp : IOneSignal, } override fun updateUserJwt( - externalId: String, + externalId: String?, token: String, ) { Logging.log(LogLevel.DEBUG, "updateUserJwt(externalId: $externalId, token: ...${token.takeLast(8)})") + if (externalId == null) throw nullParameter("updateUserJwt", "externalId") waitForInit(operationName = "updateUserJwt") @@ -813,10 +816,11 @@ internal class OneSignalImp : IOneSignal, } override suspend fun loginSuspend( - externalId: String, + externalId: String?, jwtBearerToken: String?, ) = withContext(ioDispatcher) { Logging.log(LogLevel.DEBUG, "login(externalId: $externalId, jwtBearerToken: ...${jwtBearerToken?.takeLast(8)})") + if (externalId == null) throw nullParameter("loginSuspend", "externalId") // suspendUntilInit throws on NOT_STARTED / FAILED (preserving initFailureException as the // cause), and only returns once initState == SUCCESS — so no post-check is needed here. @@ -891,10 +895,11 @@ internal class OneSignalImp : IOneSignal, wait.httpStatusCode?.let { "Login did not complete (HTTP $it)." } ?: "Login did not complete." override suspend fun updateUserJwtSuspend( - externalId: String, + externalId: String?, token: String, ) = withContext(ioDispatcher) { Logging.log(LogLevel.DEBUG, "updateUserJwtSuspend(externalId: $externalId, token: ...${token.takeLast(8)})") + if (externalId == null) throw nullParameter("updateUserJwtSuspend", "externalId") suspendUntilInit(operationName = "updateUserJwt") diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImpV2.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImpV2.kt index 435b706f8..3220641d5 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImpV2.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImpV2.kt @@ -373,7 +373,7 @@ internal class OneSignalImpV2 : IOneSignal, Logging.log(LogLevel.DEBUG, "Calling deprecated initWithContext(context: $context, appId: $appId)") if (context == null) { - Logging.error(MISSING_CONTEXT_MESSAGE) + logNullArgument("initWithContext", "context") return false } @@ -385,15 +385,21 @@ internal class OneSignalImpV2 : IOneSignal, return true } - // The only step that can throw before dispatch (the Application cast can fail in restricted - // hosts). It must end in FAILED, or IN_PROGRESS is never retired and every accessor blocks - // forever. Failures inside [internalInit] are caught there. - try { - ensureApplicationServiceStarted(context) - } catch (e: Exception) { - gate.addFailureCause(e) + // Claim this thread for the pre-dispatch work. A log listener that touches the SDK would + // otherwise wait for a job this thread has not launched. A throw must end FAILED. + val startedOk = + gate.runInit(finishIfUnfinished = false) { + try { + ensureApplicationServiceStarted(context) + true + } catch (e: Exception) { + gate.addFailureCause(e) + false + } + } + if (!startedOk) { gate.complete(InitState.FAILED) - Logging.error("initWithContext failed to start the application service", e) + Logging.error("initWithContext failed to start the application service", gate.failure) return false } @@ -414,15 +420,15 @@ internal class OneSignalImpV2 : IOneSignal, private fun failUnscheduledInit(cause: Throwable?) { val failure = IllegalStateException("initWithContext could not be scheduled: the OneSignal IO pool rejected it", cause) - Logging.error("initWithContext could not be scheduled; marking init FAILED", failure) gate.addFailureCause(failure) gate.complete(InitState.FAILED) + Logging.error("initWithContext could not be scheduled; marking init FAILED", failure) } /** * Called from internal classes only. Remain suspend until initialization is fully completed. */ - override suspend fun initWithContext(context: Context): Boolean { + override suspend fun initWithContext(context: Context?): Boolean { Logging.log(LogLevel.DEBUG, "initWithContext(context: $context)") return initWithContextSuspend(context, null) } @@ -446,15 +452,16 @@ internal class OneSignalImpV2 : IOneSignal, val result = resolveAppIdV2(appId, configModel, preferencesService) if (result.failed) { - val message = "initWithContext skipped: no valid appId was passed and none is cached. Pass your OneSignal app ID to initWithContext()." + val message = rejectedAppIdMessage(appId) gate.addFailureCause(IllegalStateException(message)) - // Null is also how background entry points (e.g. SyncJobService) start init, so it is not an app error. - if (appId == null) Logging.warn(message) else Logging.error(message) + Logging.error(message) gate.complete(InitState.FAILED) return false } - if (appId != null && !isUsableAppId(appId)) { - Logging.warn("initWithContext: appId is empty or contains a null byte, using the cached app ID instead.") + if (appId == null) { + Logging.warn("initWithContext: appId is null. Using the cached app ID.") + } else if (!isUsableAppId(appId)) { + Logging.warn("initWithContext: appId is empty or contains a null byte. Using the cached app ID.") } configModel.appId = result.appId!! // safe because failed is false val forceCreateUser = result.forceCreateUser @@ -478,14 +485,15 @@ internal class OneSignalImpV2 : IOneSignal, } override fun login( - externalId: String, + externalId: String?, jwtBearerToken: String?, ) { Logging.log(LogLevel.DEBUG, "Calling deprecated login(externalId: $externalId, jwtBearerToken: ...${jwtBearerToken?.takeLast(8)})") - if (!gate.isReady(SdkOperation.LOGIN) || isMissing(externalId, SdkOperation.LOGIN, EXTERNAL_ID_PARAM)) return + val id = requireExternalId(externalId, SdkOperation.LOGIN) + if (id == null || !gate.isReady(SdkOperation.LOGIN) || isMissing(id, "login: externalId")) return - val context = loginHelper.switchUser(externalId, jwtBearerToken).context ?: return + val context = loginHelper.switchUser(id, jwtBearerToken).context ?: return suspendifyOnIO { loginHelper.enqueueLogin(context) } } @@ -501,16 +509,16 @@ internal class OneSignalImpV2 : IOneSignal, } override fun updateUserJwt( - externalId: String, + externalId: String?, token: String, ) { Logging.log(LogLevel.DEBUG, "updateUserJwt(externalId: $externalId, token: ...${token.takeLast(8)})") - if (!gate.isReady(SdkOperation.UPDATE_USER_JWT)) return + val id = requireExternalId(externalId, SdkOperation.UPDATE_USER_JWT) + if (id == null || !gate.isReady(SdkOperation.UPDATE_USER_JWT)) return + if (isMissing(id, "updateUserJwt: externalId") || isMissing(token, "updateUserJwt: token")) return - if (isMissing(externalId, SdkOperation.UPDATE_USER_JWT, EXTERNAL_ID_PARAM) || isMissing(token, SdkOperation.UPDATE_USER_JWT, TOKEN_PARAM)) return - - jwtTokenStore.putJwt(externalId, token) + jwtTokenStore.putJwt(id, token) // Wake the queue so any deferred ops can dispatch with the fresh token. operationRepo.forceExecuteOperations() } @@ -588,7 +596,7 @@ internal class OneSignalImpV2 : IOneSignal, Logging.log(LogLevel.DEBUG, "initWithContext(context: $context, appId: $appId)") if (context == null) { - Logging.error(MISSING_CONTEXT_MESSAGE) + logNullArgument("initWithContext", "context") return false } @@ -629,16 +637,15 @@ internal class OneSignalImpV2 : IOneSignal, } override suspend fun loginSuspend( - externalId: String, + externalId: String?, jwtBearerToken: String?, ) = withContext(ioDispatcher) { Logging.log(LogLevel.DEBUG, "login(externalId: $externalId, jwtBearerToken: ...${jwtBearerToken?.takeLast(8)})") - if (!gate.awaitReady(SdkOperation.LOGIN)) return@withContext + val id = requireExternalId(externalId, SdkOperation.LOGIN) + if (id == null || !gate.awaitReady(SdkOperation.LOGIN) || isMissing(id, "login: externalId")) return@withContext - if (isMissing(externalId, SdkOperation.LOGIN, EXTERNAL_ID_PARAM)) return@withContext - - val context = loginHelper.switchUser(externalId, jwtBearerToken).context ?: return@withContext + val context = loginHelper.switchUser(id, jwtBearerToken).context ?: return@withContext loginHelper.enqueueLogin(context) } @@ -655,18 +662,18 @@ internal class OneSignalImpV2 : IOneSignal, return@withContext OneSignalResult.failure(ErrorCode.INVALID_ARGUMENT, it) } - if (!gate.awaitReady(SdkOperation.LOGIN)) { - return@withContext OneSignalResult.failure(ErrorCode.NOT_INITIALIZED, gate.notReadyMessage("login")) - } - - if (isMissing(externalId, SdkOperation.LOGIN, EXTERNAL_ID_PARAM) || externalId == null) { - return@withContext OneSignalResult.failure( - ErrorCode.INVALID_ARGUMENT, - "login: externalId is required", - ) + val id = requireExternalId(externalId, SdkOperation.LOGIN) + if (id == null || !gate.awaitReady(SdkOperation.LOGIN) || isMissing(id, "login: externalId")) { + if (id == null) { + return@withContext OneSignalResult.failure(ErrorCode.INVALID_ARGUMENT, nullArgumentMessage(SdkOperation.LOGIN.label, "externalId")) + } + if (!gate.isSuccess) { + return@withContext OneSignalResult.failure(ErrorCode.NOT_INITIALIZED, gate.notReadyMessage("login")) + } + return@withContext OneSignalResult.failure(ErrorCode.INVALID_ARGUMENT, "login: externalId is required") } - val switched = loginHelper.switchUser(externalId, jwtBearerToken, profile) + val switched = loginHelper.switchUser(id, jwtBearerToken, profile) if (switched.context != null) { val completed = loginHelper.enqueueLogin(switched.context, profile) if (!completed.success) { @@ -683,28 +690,26 @@ internal class OneSignalImpV2 : IOneSignal, return@withContext OneSignalResult.failure(error) } return@withContext OneSignalResult.success( - loginHelper.loginData(externalId, profile, completed, switched.onesignalId), + loginHelper.loginData(id, profile, completed, switched.onesignalId), ) } - OneSignalResult.success(loginHelper.loginData(externalId, profile, fallbackOnesignalId = switched.onesignalId)) + OneSignalResult.success(loginHelper.loginData(id, profile, fallbackOnesignalId = switched.onesignalId)) } private fun loginFailureMessage(wait: OperationWaitResult): String = wait.httpStatusCode?.let { "Login did not complete (HTTP $it)." } ?: "Login did not complete." override suspend fun updateUserJwtSuspend( - externalId: String, + externalId: String?, token: String, ) = withContext(ioDispatcher) { Logging.log(LogLevel.DEBUG, "updateUserJwtSuspend(externalId: $externalId, token: ...${token.takeLast(8)})") - if (!gate.awaitReady(SdkOperation.UPDATE_USER_JWT)) return@withContext - - if (isMissing(externalId, SdkOperation.UPDATE_USER_JWT, EXTERNAL_ID_PARAM) || isMissing(token, SdkOperation.UPDATE_USER_JWT, TOKEN_PARAM)) { - return@withContext - } + val id = requireExternalId(externalId, SdkOperation.UPDATE_USER_JWT) + if (id == null || !gate.awaitReady(SdkOperation.UPDATE_USER_JWT)) return@withContext + if (isMissing(id, "updateUserJwt: externalId") || isMissing(token, "updateUserJwt: token")) return@withContext - jwtTokenStore.putJwt(externalId, token) + jwtTokenStore.putJwt(id, token) operationRepo.forceExecuteOperations() } @@ -718,9 +723,38 @@ internal class OneSignalImpV2 : IOneSignal, logoutHelper.enqueueLogout(context) } - private companion object { - const val MISSING_CONTEXT_MESSAGE = "initWithContext skipped: context is required." - const val EXTERNAL_ID_PARAM = "externalId" - const val TOKEN_PARAM = "token" + private fun requireExternalId( + externalId: String?, + operation: SdkOperation, + ): String? { + if (externalId != null) return externalId + logNullArgument(operation.label, "externalId") + return null + } + + private fun logNullArgument( + method: String, + parameter: String, + ) { + Logging.error(nullArgumentMessage(method, parameter)) + } + + // isInitialized is false while init is in progress, which is the state a rejected init leaves behind. + private fun nullArgumentMessage( + method: String, + parameter: String, + ): String { + val problem = "$method: $parameter is null, so this call did not run." + return if (isInitialized) problem else "$problem The SDK is not initialized." + } + + private fun rejectedAppIdMessage(appId: String?): String { + val problem = + when { + appId == null -> "initWithContext: appId is null and no app ID is cached" + appId.isEmpty() -> "initWithContext: appId is empty and no app ID is cached" + else -> "initWithContext: appId contains a null byte and no app ID is cached" + } + return "$problem, so initialization did not finish. The SDK is not initialized." } } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/InputGuardTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/InputGuardTests.kt index 40f8630d3..ae2957658 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/InputGuardTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/InputGuardTests.kt @@ -2,7 +2,6 @@ package com.onesignal.common import com.onesignal.debug.LogLevel import com.onesignal.debug.internal.logging.Logging -import com.onesignal.internal.SdkOperation import io.kotest.core.spec.style.FunSpec import io.kotest.matchers.shouldBe @@ -25,11 +24,4 @@ class InputGuardTests : FunSpec({ isMissing("abc\u0000", "addAlias: id") shouldBe true hasMissingEntries(mapOf("external_id" to "\u0000: 1"), "addAliases", allowEmptyValue = false) shouldBe true } - - test("the operation overload applies the same checks and logs the public API name") { - isMissing(null, SdkOperation.LOGIN, "externalId") shouldBe true - isMissing("\u0000", SdkOperation.LOGIN, "externalId") shouldBe true - isMissing("user-1", SdkOperation.LOGIN, "externalId") shouldBe false - "${SdkOperation.LOGIN}: externalId" shouldBe "login: externalId" - } }) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/OneSignalImpParityTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/OneSignalImpParityTests.kt new file mode 100644 index 000000000..18129d729 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/OneSignalImpParityTests.kt @@ -0,0 +1,169 @@ +package com.onesignal.core.internal.application + +import android.content.Context +import androidx.test.core.app.ApplicationProvider.getApplicationContext +import br.com.colman.kotest.android.extensions.robolectric.RobolectricTest +import com.onesignal.IOneSignal +import com.onesignal.common.services.IServiceProvider +import com.onesignal.core.internal.config.ConfigModelStore +import com.onesignal.debug.LogLevel +import com.onesignal.debug.internal.logging.Logging +import com.onesignal.internal.OneSignalImp +import com.onesignal.internal.OneSignalImpV2 +import io.kotest.assertions.withClue +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe + +/** + * Runs the same scenario on the legacy and the never-crash implementation and compares what the app + * can observe. The only intended differences are the calls that used to throw, which are covered by + * the V2 suites; everything here must stay identical. + */ +@RobolectricTest +class OneSignalImpParityTests : FunSpec({ + + fun IOneSignal.awaitInit(timeoutMs: Long = 5_000): Boolean { + val deadline = System.currentTimeMillis() + timeoutMs + while (!isInitialized && System.currentTimeMillis() < deadline) Thread.sleep(10) + return isInitialized + } + + fun IOneSignal.awaitExternalId(expected: String, timeoutMs: Long = 5_000): String { + val deadline = System.currentTimeMillis() + timeoutMs + while (user.externalId != expected && System.currentTimeMillis() < deadline) Thread.sleep(10) + return user.externalId + } + + fun IOneSignal.appIdInConfig() = (this as IServiceProvider).getService(ConfigModelStore::class.java).model.appId + + // PreferencesService flushes on a 200ms buffer, so an instance from the previous scenario can still + // write after a clear; let it land first. + fun clearPrefs() { + Thread.sleep(400) + val context = getApplicationContext() + for (name in listOf("OneSignal", "com.onesignal")) { + context.getSharedPreferences(name, Context.MODE_PRIVATE).edit().clear().commit() + } + } + + /** Inits a throwaway instance of the same implementation and waits until its app ID is on disk. */ + fun IOneSignal.cacheAppId(context: Context, appId: String) { + val warm = if (this is OneSignalImpV2) OneSignalImpV2() else OneSignalImp() + Logging.logLevel = LogLevel.NONE + warm.initWithContext(context, appId) + warm.awaitInit() shouldBe true + val prefs = context.getSharedPreferences("OneSignal", Context.MODE_PRIVATE) + val deadline = System.currentTimeMillis() + 5_000 + while (prefs.all.values.none { it.toString().contains(appId) } && System.currentTimeMillis() < deadline) Thread.sleep(10) + prefs.all.values.any { it.toString().contains(appId) } shouldBe true + } + + fun implementations(): List IOneSignal>> = + listOf( + "legacy" to { OneSignalImp().also { Logging.logLevel = LogLevel.NONE } }, + "v2" to { OneSignalImpV2().also { Logging.logLevel = LogLevel.NONE } }, + ) + + /** Runs [scenario] on a fresh instance of each implementation, with clean prefs, and compares the results. */ + fun parity(scenario: (IOneSignal, Context) -> Any?) { + val results = + implementations().map { (name, create) -> + clearPrefs() + name to scenario(create(), getApplicationContext()) + } + withClue("legacy=${results[0].second} v2=${results[1].second}") { + results[1].second shouldBe results[0].second + } + } + + beforeAny { Logging.logLevel = LogLevel.NONE } + afterAny { clearPrefs() } + + test("init then reading every top-level value") { + parity { os, context -> + os.initWithContext(context, "appId") + os.awaitInit() shouldBe true + os.isInitialized shouldBe true + os.appIdInConfig() shouldBe "appId" + listOf( + os.isInitialized, + os.user.onesignalId.isNotEmpty(), + os.user.externalId, + os.user.pushSubscription.optedIn, + os.user.pushSubscription.id, + os.user.getTags(), + os.consentRequired, + os.consentGiven, + os.disableGMSMissingPrompt, + os.appIdInConfig(), + ) + } + } + + test("settings changed before init reach the config model") { + parity { os, context -> + os.consentRequired = true + os.disableGMSMissingPrompt = true + val beforeInit = listOf(os.consentRequired, os.consentGiven, os.disableGMSMissingPrompt) + os.initWithContext(context, "appId") + os.awaitInit() shouldBe true + os.isInitialized shouldBe true + val model = (os as IServiceProvider).getService(ConfigModelStore::class.java).model + beforeInit + listOf(os.consentRequired, os.consentGiven, os.disableGMSMissingPrompt, model.consentRequired, model.consentGiven, model.disableGMSMissingPrompt) + } + } + + test("a second initWithContext after success keeps the first app ID") { + parity { os, context -> + os.initWithContext(context, "appId") + os.awaitInit() shouldBe true + os.isInitialized shouldBe true + os.appIdInConfig() shouldBe "appId" + val second = os.initWithContext(context, "otherAppId") + Thread.sleep(100) + listOf(second, os.isInitialized, os.appIdInConfig()) + } + } + + test("login, tags and logout after init") { + parity { os, context -> + os.initWithContext(context, "appId") + os.awaitInit() shouldBe true + os.isInitialized shouldBe true + os.login("ext-1") + val afterLogin = os.awaitExternalId("ext-1") + afterLogin shouldBe "ext-1" + os.user.addTags(mapOf("a" to "1", "b" to "2")) + os.user.removeTag("b") + val tags = os.user.getTags() + os.logout() + val afterLogout = os.awaitExternalId("") + listOf(afterLogin, tags, afterLogout, os.user.onesignalId.isNotEmpty()) + } + } + + test("a cached app ID is reused when init is called with an empty app ID") { + parity { os, context -> + os.cacheAppId(context, "cachedAppId") + + os.initWithContext(context, "") + os.awaitInit() shouldBe true + os.isInitialized shouldBe true + os.appIdInConfig() shouldBe "cachedAppId" + listOf(os.isInitialized, os.appIdInConfig()) + } + } + + test("init with a null app ID resolves from the cache written by an earlier init") { + parity { os, context -> + os.cacheAppId(context, "cachedAppId") + + val result = + kotlinx.coroutines.runBlocking { os.initWithContextSuspend(context, null) } + result shouldBe true + os.isInitialized shouldBe true + os.appIdInConfig() shouldBe "cachedAppId" + listOf(result, os.isInitialized, os.appIdInConfig()) + } + } +}) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/SDKInitV2Tests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/SDKInitV2Tests.kt index 47142b83e..c1a3dd310 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/SDKInitV2Tests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/SDKInitV2Tests.kt @@ -289,7 +289,7 @@ class SDKInitV2Tests : FunSpec({ os.isInitialized shouldBe true events.filter { it.level == LogLevel.ERROR && it.entry.contains("appId") } shouldBe emptyList() - events.count { it.level == LogLevel.WARN && it.entry.contains("using the cached app ID") } shouldBe 1 + events.count { it.level == LogLevel.WARN && it.entry.contains("Using the cached app ID") } shouldBe 1 } test("an ignored call logs one error per attempt, with the failure trace, and repeats at debug") { diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/DeferredManagersCoverageTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/DeferredManagersCoverageTests.kt new file mode 100644 index 000000000..3bb859af2 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/DeferredManagersCoverageTests.kt @@ -0,0 +1,172 @@ +package com.onesignal.internal + +import com.onesignal.inAppMessages.IInAppMessagesManager +import com.onesignal.location.ILocationManager +import com.onesignal.notifications.INotificationsManager +import com.onesignal.session.ISessionManager +import com.onesignal.user.IUserManager +import com.onesignal.user.subscriptions.IPushSubscription +import io.kotest.assertions.withClue +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe +import io.mockk.mockk +import io.mockk.mockkClass +import io.mockk.verify +import kotlin.reflect.KCallable +import kotlin.reflect.KClass +import kotlin.reflect.KFunction2 +import kotlin.reflect.KMutableProperty +import kotlin.reflect.KMutableProperty1 +import kotlin.reflect.full.declaredMembers + +private class NotReadySource : ManagerSource { + override fun get(member: String): T? = null + + override fun peek(): T? = null + + override suspend fun await(member: String): T? = null + + override fun logQueued(member: String) = Unit +} + +private val standIns: Map, KClass<*>> = + mapOf( + IUserManager::class to DeferredUserManager::class, + IPushSubscription::class to DeferredPushSubscription::class, + INotificationsManager::class to DeferredNotificationsManager::class, + IInAppMessagesManager::class to DeferredInAppMessagesManager::class, + ILocationManager::class to DeferredLocationManager::class, + ISessionManager::class to DeferredSessionManager::class, + ) + +private class ReplayCase( + val type: KClass, + val member: String, + val standIn: (ManagerSource) -> DeferredManager, + val call: T.() -> Unit, +) { + fun run() { + val real = mockkClass(type, relaxed = true) + val deferred = standIn(NotReadySource()) + @Suppress("UNCHECKED_CAST") + (deferred as T).call() + verify(exactly = 0) { real.call() } + deferred.replay(real) + verify(exactly = 1) { real.call() } + } +} + +// One listener per case, so the replayed call can be verified against the same instance. +private inline fun registration( + noinline standIn: (ManagerSource) -> DeferredManager, + add: KFunction2, +): ReplayCase { + val listener = mockk(relaxed = true) + return ReplayCase(T::class, add.name, standIn) { add(this, listener) } +} + +private inline fun setting( + noinline standIn: (ManagerSource) -> DeferredManager, + property: KMutableProperty1, +) = ReplayCase(T::class, property.name, standIn) { property.set(this, true) } + +private val replayCases: List> = + listOf( + registration(::DeferredUserManager, IUserManager::addObserver), + registration(::DeferredPushSubscription, IPushSubscription::addObserver), + registration(::DeferredNotificationsManager, INotificationsManager::addPermissionObserver), + registration(::DeferredNotificationsManager, INotificationsManager::addForegroundLifecycleListener), + registration(::DeferredNotificationsManager, INotificationsManager::addClickListener), + registration(::DeferredInAppMessagesManager, IInAppMessagesManager::addLifecycleListener), + registration(::DeferredInAppMessagesManager, IInAppMessagesManager::addClickListener), + setting(::DeferredInAppMessagesManager, IInAppMessagesManager::paused), + setting(::DeferredLocationManager, ILocationManager::isShared), + ) + +private val replayedMember = Regex("^add\\w*(Listener|Observer)$") +private val removedMember = Regex("^remove\\w*(Listener|Observer)$") + +private class RemoveCase( + val type: KClass, + val member: String, + val standIn: (ManagerSource) -> DeferredManager, + val add: T.() -> Unit, + val remove: T.() -> Unit, +) { + fun run() { + val real = mockkClass(type, relaxed = true) + val deferred = standIn(NotReadySource()) + @Suppress("UNCHECKED_CAST") + (deferred as T).add() + @Suppress("UNCHECKED_CAST") + (deferred as T).remove() + deferred.replay(real) + verify(exactly = 0) { real.add() } + } +} + +private inline fun removal( + noinline standIn: (ManagerSource) -> DeferredManager, + add: KFunction2, + remove: KFunction2, +): RemoveCase { + val listener = mockk(relaxed = true) + return RemoveCase(T::class, remove.name, standIn, { add(this, listener) }, { remove(this, listener) }) +} + +private val removeCases: List> = + listOf( + removal(::DeferredUserManager, IUserManager::addObserver, IUserManager::removeObserver), + removal(::DeferredPushSubscription, IPushSubscription::addObserver, IPushSubscription::removeObserver), + removal(::DeferredNotificationsManager, INotificationsManager::addPermissionObserver, INotificationsManager::removePermissionObserver), + removal(::DeferredNotificationsManager, INotificationsManager::addForegroundLifecycleListener, INotificationsManager::removeForegroundLifecycleListener), + removal(::DeferredNotificationsManager, INotificationsManager::addClickListener, INotificationsManager::removeClickListener), + removal(::DeferredInAppMessagesManager, IInAppMessagesManager::addLifecycleListener, IInAppMessagesManager::removeLifecycleListener), + removal(::DeferredInAppMessagesManager, IInAppMessagesManager::addClickListener, IInAppMessagesManager::removeClickListener), + ) + +private fun KCallable<*>.signature() = "$name(${parameters.drop(1).joinToString { it.type.toString() }})" + +private fun KClass<*>.apiMembers() = members.filter { it.name !in setOf("equals", "hashCode", "toString") } + +class DeferredManagersCoverageTests : FunSpec({ + test("every interface member is implemented on its stand-in, not inherited from a default body") { + val missing = + standIns.flatMap { (iface, standIn) -> + val declared = standIn.declaredMembers.map { it.signature() }.toSet() + iface.apiMembers().map { it.signature() }.filter { it !in declared } + .map { "${iface.simpleName}.$it: implement it in ${standIn.simpleName}" } + } + missing shouldBe emptyList() + } + + test("every listener, observer and setting has a replay case") { + val covered = replayCases.map { "${it.type.simpleName}.${it.member}" }.toSet() + val uncovered = + standIns.keys.flatMap { iface -> + iface.apiMembers().filter { it is KMutableProperty<*> || replayedMember.matches(it.name) } + .map { "${iface.simpleName}.${it.name}" } + .filter { it !in covered } + } + withClue("decide whether each queues before init, then add it to replayCases") { uncovered shouldBe emptyList() } + } + + test("each queued registration or setting reaches the real manager on replay") { + replayCases.forEach { case -> withClue("${case.type.simpleName}.${case.member}") { case.run() } } + } + + test("every remove has an add-then-remove case") { + val covered = removeCases.map { "${it.type.simpleName}.${it.member}" }.toSet() + val uncovered = + standIns.keys.flatMap { iface -> + iface.apiMembers().filter { removedMember.matches(it.name) } + .map { "${iface.simpleName}.${it.name}" } + .filter { it !in covered } + } + withClue("pair each add with an add-then-remove that expects the real manager to see neither") { uncovered shouldBe emptyList() } + } + + test("a remove before replay cancels the queued add") { + removeCases.forEach { case -> withClue("${case.type.simpleName}.${case.member}") { case.run() } } + } +}) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/DeferredManagersTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/DeferredManagersTests.kt index c4d451273..06d53272d 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/DeferredManagersTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/DeferredManagersTests.kt @@ -26,6 +26,9 @@ import io.mockk.mockk import io.mockk.verify import io.mockk.verifyOrder import kotlinx.coroutines.runBlocking +import java.util.Collections +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit private class FakeSource(private val real: T) : ManagerSource { var ready = false @@ -330,6 +333,36 @@ class DeferredManagersTests : FunSpec({ deferred.paused shouldBe true } + test("a remove that arrives while its add is being replayed runs after that add") { + val real = mockk(relaxed = true) + val deferred = DeferredNotificationsManager(FakeSource(real)) + val listener = mockk() + val addEntered = CountDownLatch(1) + val removeFinished = CountDownLatch(1) + val order = Collections.synchronizedList(mutableListOf()) + every { real.addClickListener(listener) } answers { + addEntered.countDown() + check(removeFinished.await(5, TimeUnit.SECONDS)) + order.add("add-done") + } + every { real.removeClickListener(listener) } answers { order.add("remove") } + deferred.addClickListener(listener) + + val remover = + Thread { + check(addEntered.await(5, TimeUnit.SECONDS)) + deferred.removeClickListener(listener) + removeFinished.countDown() + } + remover.isDaemon = true + remover.start() + deferred.replay(real) + remover.join(5_000) + + remover.isAlive shouldBe false + order shouldBe listOf("add-done", "remove") + } + test("a listener that registers another while it is replayed reaches the real manager without waiting") { val real = mockk(relaxed = true) every { real.pushSubscription } returns mockk(relaxed = true) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/NeverCrashKillSwitchTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/NeverCrashKillSwitchTests.kt index a94209cc7..7fa17eccb 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/NeverCrashKillSwitchTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/NeverCrashKillSwitchTests.kt @@ -38,48 +38,48 @@ class NeverCrashKillSwitchTests : FunSpec({ afterEach { NeverCrashKillSwitch.resetForTest() } - test("the cached kill switch selects the legacy implementation") { + test("the cached flag selects the never-crash implementation") { cacheFlags("other_flag", NeverCrashKillSwitch.FLAG_KEY) NeverCrashKillSwitch.evaluate(getApplicationContext()) - NeverCrashKillSwitch.useLegacy shouldBe true - NeverCrashKillSwitch.create().shouldBeInstanceOf() + NeverCrashKillSwitch.useLegacy shouldBe false + NeverCrashKillSwitch.create().shouldBeInstanceOf() } - test("the cached kill switch matches regardless of wire casing") { + test("the cached flag matches regardless of wire casing") { cacheFlags(NeverCrashKillSwitch.FLAG_KEY.uppercase()) NeverCrashKillSwitch.evaluate(getApplicationContext()) - NeverCrashKillSwitch.useLegacy shouldBe true + NeverCrashKillSwitch.useLegacy shouldBe false } - test("without the kill switch the never-crash implementation is used") { + test("without the flag the legacy implementation is used") { cacheFlags("other_flag") NeverCrashKillSwitch.evaluate(getApplicationContext()) - NeverCrashKillSwitch.useLegacy shouldBe false - NeverCrashKillSwitch.create().shouldBeInstanceOf() + NeverCrashKillSwitch.useLegacy shouldBe true + NeverCrashKillSwitch.create().shouldBeInstanceOf() } - test("a fresh install with nothing cached uses the never-crash implementation") { + test("a fresh install with nothing cached uses the legacy implementation") { NeverCrashKillSwitch.evaluate(getApplicationContext()) - NeverCrashKillSwitch.useLegacy shouldBe false + NeverCrashKillSwitch.useLegacy shouldBe true } - test("an unreadable cached config keeps the never-crash implementation without throwing") { + test("an unreadable cached config keeps the legacy implementation without throwing") { prefs().edit().putString(configKey, "{not json").commit() NeverCrashKillSwitch.evaluate(getApplicationContext()) NeverCrashKillSwitch.evaluated shouldBe true - NeverCrashKillSwitch.useLegacy shouldBe false + NeverCrashKillSwitch.useLegacy shouldBe true } - test("a locked device keeps the never-crash implementation") { + test("a locked device keeps the legacy implementation") { cacheFlags(NeverCrashKillSwitch.FLAG_KEY) mockkObject(AndroidUtils) try { @@ -87,13 +87,13 @@ class NeverCrashKillSwitchTests : FunSpec({ NeverCrashKillSwitch.evaluate(getApplicationContext()) - NeverCrashKillSwitch.useLegacy shouldBe false + NeverCrashKillSwitch.useLegacy shouldBe true } finally { unmockkObject(AndroidUtils) } } - test("first use before the switch is evaluated uses the never-crash implementation") { - NeverCrashKillSwitch.create().shouldBeInstanceOf() + test("first use before the flag is evaluated uses the legacy implementation") { + NeverCrashKillSwitch.create().shouldBeInstanceOf() } }) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpTests.kt index fb71ff5d3..365759be4 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpTests.kt @@ -451,6 +451,12 @@ class OneSignalImpTests : FunSpec({ "Parameter specified as non-null is null: method com.onesignal.OneSignal.initWithContext, parameter appId" shouldThrowUnit { runBlocking { os.initWithContextSuspend(null, "appId") } }.message shouldBe "Parameter specified as non-null is null: method com.onesignal.OneSignal.initWithContextSuspend, parameter context" + shouldThrowUnit { runBlocking { os.initWithContext(null) } }.message shouldBe + "Parameter specified as non-null is null: method com.onesignal.OneSignal.initWithContext, parameter context" + shouldThrowUnit { os.login(null) }.message shouldBe + "Parameter specified as non-null is null: method com.onesignal.OneSignal.login, parameter externalId" + shouldThrowUnit { os.updateUserJwt(null, "token") }.message shouldBe + "Parameter specified as non-null is null: method com.onesignal.OneSignal.updateUserJwt, parameter externalId" os.isInitialized shouldBe false } }) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2ApiSweepTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2ApiSweepTests.kt new file mode 100644 index 000000000..982c2fca8 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2ApiSweepTests.kt @@ -0,0 +1,170 @@ +package com.onesignal.internal + +import android.content.Context +import com.onesignal.IOneSignal +import com.onesignal.OneSignalUserProfile +import com.onesignal.debug.LogLevel +import com.onesignal.debug.internal.logging.Logging +import com.onesignal.inAppMessages.IInAppMessagesManager +import com.onesignal.location.ILocationManager +import com.onesignal.notifications.INotificationsManager +import com.onesignal.session.ISessionManager +import com.onesignal.user.IUserManager +import com.onesignal.user.subscriptions.IPushSubscription +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe +import io.mockk.mockkClass +import kotlinx.coroutines.runBlocking +import java.lang.reflect.InvocationTargetException +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit +import java.util.concurrent.TimeoutException +import kotlin.reflect.KCallable +import kotlin.reflect.KClass +import kotlin.reflect.KFunction +import kotlin.reflect.KMutableProperty +import kotlin.reflect.KParameter +import kotlin.reflect.KProperty +import kotlin.reflect.KType +import kotlin.reflect.full.callSuspend + +private fun dummy(type: KType): Any? { + val kClass = type.classifier as KClass<*> + // A nullable string still has to reach the gate. A nullable Context would start a real init. + if (type.isMarkedNullable && kClass != String::class) return null + return when { + kClass == String::class -> "sample" + kClass == Boolean::class -> false + kClass == Int::class -> 0 + kClass == Float::class -> 0f + kClass == Map::class -> emptyMap() + kClass == Collection::class -> emptyList() + kClass == OneSignalUserProfile::class -> OneSignalUserProfile.builder().build() + kClass.java.isEnum -> kClass.java.enumConstants!!.first() + else -> mockkClass(kClass, relaxed = true) + } +} + +private fun KCallable<*>.label() = "$name(${parameters.drop(1).joinToString { it.type.toString() }})" + +// Initializing with a real context is SDKInitV2Tests' job; here only the null-context variants run. +private fun KCallable<*>.startsInit() = parameters.any { it.type.classifier == Context::class && !it.type.isMarkedNullable } + +private fun KCallable<*>.isApi() = name !in setOf("equals", "hashCode", "toString") && !startsInit() + +private val sweepThread = Executors.newCachedThreadPool { r -> Thread(r).also { it.isDaemon = true } } + +// A blocking wait inside the member cannot be interrupted by withTimeout, so the deadline is enforced from outside. +private fun callWithDeadline(block: () -> Unit) { + val future = sweepThread.submit(block) + try { + future.get(5, TimeUnit.SECONDS) + } catch (e: java.util.concurrent.ExecutionException) { + throw e.cause ?: e + } +} + +private fun invoke( + member: KCallable<*>, + target: Any, + args: (KParameter) -> Any? = { dummy(it.type) }, +) { + runBlocking { + when (member) { + is KProperty<*> -> { + member.getter.call(target) + if (member is KMutableProperty<*>) member.setter.call(target, dummy(member.returnType)) + } + is KFunction<*> -> member.callSuspend(target, *member.parameters.drop(1).map(args).toTypedArray()) + else -> Unit + } + Unit + } +} + +// Calls every member of [type] on [target()] with dummy args; returns the ones that threw or hung. +private fun sweep( + type: KClass<*>, + target: () -> Any, +): List = + type.members.filter { it.isApi() }.mapNotNull { member -> + try { + callWithDeadline { invoke(member, target()) } + null + } catch (e: TimeoutException) { + "${type.simpleName}.${member.label()} hung for 5s" + } catch (e: Throwable) { + "${type.simpleName}.${member.label()} threw ${(e as? InvocationTargetException)?.targetException ?: e}" + } + } + +private fun KParameter.isRequiredReference() = !type.isMarkedNullable && !(type.classifier as KClass<*>).java.isPrimitive && type.classifier != Boolean::class && type.classifier != Int::class && type.classifier != Float::class + +// Passes null for each required parameter in turn; returns the calls that hung or threw anything but a NullPointerException. +private fun nullSweep( + type: KClass<*>, + target: () -> Any, +): List = + type.members.filterIsInstance>().filter { it.isApi() }.flatMap { member -> + member.parameters.drop(1).filter { it.isRequiredReference() }.mapNotNull { nulled -> + val outcome = + try { + callWithDeadline { invoke(member, target()) { if (it == nulled) null else dummy(it.type) } } + null + } catch (e: TimeoutException) { + "hung for 5s" + } catch (e: Throwable) { + val cause = (e as? InvocationTargetException)?.targetException ?: e + if (cause is NullPointerException) null else "threw $cause" + } + outcome?.let { "${type.simpleName}.${member.label()} with null ${nulled.name} $it" } + } + } + +// DebugManager's init resets the level to WARN, and WARN reaches the unmocked android.util.Log. +private fun quietOneSignal() = OneSignalImpV2().also { Logging.logLevel = LogLevel.NONE } + +private fun failedOneSignal() = + quietOneSignal().also { + it.gate.tryStart() + it.gate.complete(InitState.FAILED) + } + +private fun managers(os: OneSignalImpV2): List, () -> Any>> = + listOf( + IUserManager::class to { os.user }, + IPushSubscription::class to { os.user.pushSubscription }, + INotificationsManager::class to { os.notifications }, + IInAppMessagesManager::class to { os.inAppMessages }, + ILocationManager::class to { os.location }, + ISessionManager::class to { os.session }, + ) + +class OneSignalImpV2ApiSweepTests : FunSpec({ + test("no IOneSignal member throws or hangs before init") { + sweep(IOneSignal::class) { quietOneSignal() } shouldBe emptyList() + } + + test("no member of a manager handed out before init throws or hangs") { + val os = quietOneSignal() + managers(os).flatMap { (type, target) -> sweep(type, target) } shouldBe emptyList() + } + + test("no IOneSignal member throws or hangs after a failed init") { + sweep(IOneSignal::class) { failedOneSignal() } shouldBe emptyList() + } + + test("no member of a manager handed out after a failed init throws or hangs") { + val os = failedOneSignal() + managers(os).flatMap { (type, target) -> sweep(type, target) } shouldBe emptyList() + } + + // Java callers passing null for a still-required argument hit Kotlin's parameter check. externalId, + // context, and appId are nullable and log instead. Either way the call must not hang. + test("a null argument for a required parameter never hangs and throws nothing but a NullPointerException") { + val before = quietOneSignal() + val failed = failedOneSignal() + (listOf(IOneSignal::class to { before }) + managers(before)).flatMap { (type, target) -> nullSweep(type, target) } shouldBe emptyList() + (listOf(IOneSignal::class to { failed }) + managers(failed)).flatMap { (type, target) -> nullSweep(type, target) } shouldBe emptyList() + } +}) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2FaultTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2FaultTests.kt index 0980494d0..7494fe196 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2FaultTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2FaultTests.kt @@ -96,9 +96,14 @@ class OneSignalImpV2FaultTests : FunSpec({ ioExecutor.get()!!.queue.remainingCapacity() shouldBe 0 val os = quietOneSignal() - os.initWithContext(getApplicationContext(), "appId") - - awaitTerminal(os) shouldBe InitState.FAILED + val listener = ILogListener { if (it.entry.contains("could not be scheduled")) os.user.onesignalId } + Logging.addListener(listener) + try { + os.initWithContext(getApplicationContext(), "appId") + awaitTerminal(os) shouldBe InitState.FAILED + } finally { + Logging.removeListener(listener) + } onDaemonThread { os.user.onesignalId } shouldBe "" os.isInitialized shouldBe false } finally { @@ -112,6 +117,34 @@ class OneSignalImpV2FaultTests : FunSpec({ awaitTerminal(retry) shouldBe InitState.SUCCESS } + test("a log listener that touches the SDK while init is starting does not block the dispatch") { + val os = quietOneSignal() + val listener = ILogListener { if (it.entry.contains("ApplicationService.init")) os.user.onesignalId } + Logging.addListener(listener) + try { + onDaemonThread { os.initWithContext(getApplicationContext(), "appId") } + awaitTerminal(os) shouldBe InitState.SUCCESS + } finally { + Logging.removeListener(listener) + } + } + + test("a LinkageError while starting the application service marks init FAILED") { + val os = quietOneSignal() + val listener = + ILogListener { + if (it.entry.contains("ApplicationService.init")) throw NoClassDefFoundError("missing") + } + Logging.addListener(listener) + try { + onDaemonThread { os.initWithContext(getApplicationContext(), "appId") } shouldBe false + os.gate.state shouldBe InitState.FAILED + onDaemonThread { os.user.onesignalId } shouldBe "" + } finally { + Logging.removeListener(listener) + } + } + test("a log listener that calls back into the SDK before init does not recurse until the stack overflows") { val os = quietOneSignal() val reentries = AtomicInteger() diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2Tests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2Tests.kt index b52b92bfa..f0186a9b6 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2Tests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/OneSignalImpV2Tests.kt @@ -1,9 +1,12 @@ package com.onesignal.internal +import com.onesignal.debug.ILogListener import com.onesignal.debug.LogLevel import com.onesignal.debug.internal.logging.Logging import io.kotest.core.spec.style.FunSpec import io.kotest.matchers.shouldBe +import io.kotest.matchers.string.shouldContain +import io.kotest.matchers.string.shouldNotContain class OneSignalImpV2Tests : FunSpec({ beforeAny { @@ -22,17 +25,43 @@ class OneSignalImpV2Tests : FunSpec({ os.login("login-id") os.login("") + os.login(null) + os.updateUserJwt(null, "token") os.isInitialized shouldBe false } - test("login with empty externalId after init does not switch user") { + test("a null context or externalId is logged, and the log says the SDK is not initialized") { + val os = OneSignalImpV2() + val entries = mutableListOf() + val listener = ILogListener { entries.add(it.entry) } + Logging.addListener(listener) + Logging.logLevel = LogLevel.NONE + try { + os.initWithContext(null, "app-id") shouldBe false + os.login(null) + entries.single { "context is null" in it } shouldContain "The SDK is not initialized." + entries.single { "login: externalId is null" in it } shouldContain "The SDK is not initialized." + + entries.clear() + markInitialized(os) + os.login(null) + val afterInit = entries.single { "login: externalId is null" in it } + afterInit shouldContain "so this call did not run." + afterInit shouldNotContain "The SDK is not initialized." + } finally { + Logging.removeListener(listener) + } + } + + test("login with a null or empty externalId after init does not switch user") { val os = OneSignalImpV2() markInitialized(os) // NONE: otherwise Logging.error calls the unmocked android.util.Log. Logging.logLevel = LogLevel.NONE os.login("") + os.login(null) } test("login with a null byte in externalId after init does not switch user") { @@ -49,6 +78,7 @@ class OneSignalImpV2Tests : FunSpec({ Logging.logLevel = LogLevel.NONE os.updateUserJwt("", "token") + os.updateUserJwt(null, "token") os.updateUserJwt("user", "") }