Merge pull request #2157 from OneSignal/fix/null_onesignalid_crash

Recover null onesignal ID crashes for Operations

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt

@@ -38,8 +38,7 @@ internal class OperationModelStore(prefs: IPreferencesService) : ModelStore<Oper
             return null
         }
 
-        if (!jsonObject.has(Operation::name.name)) {
-            Logging.error("jsonObject must have '${Operation::name.name}' attribute")
+        if (!isValidOperation(jsonObject)) {
             return null
         }
 
@@ -69,4 +68,34 @@ internal class OperationModelStore(prefs: IPreferencesService) : ModelStore<Oper
 
         return operation
     }
+
+    /**
+     * Checks if a JSONObject is a valid Operation. Contains a check for onesignalId.
+     * This is a rare case that a cached Operation is missing the onesignalId,
+     * which would continuously cause crashes when the Operation is processed.
+     *
+     * @param jsonObject The [JSONObject] that represents an Operation
+     */
+    private fun isValidOperation(jsonObject: JSONObject): Boolean {
+        if (!jsonObject.has(Operation::name.name)) {
+            Logging.error("jsonObject must have '${Operation::name.name}' attribute")
+            return false
+        }
+
+        val operationName = jsonObject.getString(Operation::name.name)
+
+        val excluded =
+            setOf(
+                LoginUserOperationExecutor.LOGIN_USER,
+                LoginUserFromSubscriptionOperationExecutor.LOGIN_USER_FROM_SUBSCRIPTION_USER,
+            )
+
+        // Must have onesignalId if it is not one of the excluded operations above
+        if (!jsonObject.has("onesignalId") && !excluded.contains(operationName)) {
+            Logging.error("$operationName jsonObject must have 'onesignalId' attribute")
+            return false
+        }
+
+        return true
+    }
 }

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ModelingTests.kt

@@ -9,8 +9,8 @@ import com.onesignal.core.internal.preferences.PreferenceOneSignalKeys
 import com.onesignal.core.internal.preferences.PreferenceStores
 import com.onesignal.mocks.MockHelper
 import com.onesignal.mocks.MockPreferencesService
-import com.onesignal.user.internal.operations.SetPropertyOperation
-import com.onesignal.user.internal.operations.SetTagOperation
+import com.onesignal.user.internal.operations.LoginUserFromSubscriptionOperation
+import com.onesignal.user.internal.operations.LoginUserOperation
 import com.onesignal.user.internal.subscriptions.SubscriptionModel
 import com.onesignal.user.internal.subscriptions.SubscriptionModelStore
 import io.kotest.core.spec.style.FunSpec
@@ -150,15 +150,15 @@ class ModelingTests : FunSpec({
         val operationModelStore = OperationModelStore(prefs)
         val jsonArray = JSONArray()
 
-        val cachedOperation = SetTagOperation()
+        val cachedOperation = LoginUserFromSubscriptionOperation()
         cachedOperation.id = UUID.randomUUID().toString()
         // Add duplicate operations to the cache
         jsonArray.put(cachedOperation.toJSON())
         jsonArray.put(cachedOperation.toJSON())
         prefs.saveString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + "operations", jsonArray.toString())
 
         // When - adding an operation first and then loading from cache
-        val newOperation = SetPropertyOperation()
+        val newOperation = LoginUserOperation()
         newOperation.id = UUID.randomUUID().toString()
         operationModelStore.add(newOperation)
         operationModelStore.loadOperations()

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationModelStoreTests.kt

@@ -0,0 +1,66 @@
+package com.onesignal.core.internal.operations
+
+import com.onesignal.core.internal.operations.impl.OperationModelStore
+import com.onesignal.core.internal.preferences.PreferenceOneSignalKeys
+import com.onesignal.core.internal.preferences.PreferenceStores
+import com.onesignal.debug.LogLevel
+import com.onesignal.debug.internal.logging.Logging
+import com.onesignal.mocks.MockPreferencesService
+import com.onesignal.user.internal.operations.LoginUserOperation
+import com.onesignal.user.internal.operations.SetPropertyOperation
+import io.kotest.core.spec.style.FunSpec
+import io.kotest.matchers.shouldBe
+import io.kotest.matchers.shouldNotBe
+import org.json.JSONArray
+import org.json.JSONObject
+import java.util.UUID
+
+class OperationModelStoreTests : FunSpec({
+
+    beforeAny {
+        Logging.logLevel = LogLevel.NONE
+    }
+
+    test("does not load invalid cached operations") {
+        // Given
+        val prefs = MockPreferencesService()
+        val operationModelStore = OperationModelStore(prefs)
+        val jsonArray = JSONArray()
+
+        // 1. Create a VALID Operation with onesignalId
+        val validOperation = SetPropertyOperation(UUID.randomUUID().toString(), UUID.randomUUID().toString(), "property", "value")
+        validOperation.id = UUID.randomUUID().toString()
+
+        // 2. Create a VALID operation missing onesignalId
+        val validOperationMissingOnesignalId = LoginUserOperation()
+        validOperationMissingOnesignalId.id = UUID.randomUUID().toString()
+
+        // 3. Create an INVALID Operation missing onesignalId
+        val invalidOperationMissingOnesignalId = SetPropertyOperation()
+        invalidOperationMissingOnesignalId.id = UUID.randomUUID().toString()
+
+        // 4. Create an INVALID Operation missing operation name
+        val invalidOperationMissingName =
+            JSONObject()
+                .put("app_id", UUID.randomUUID().toString())
+                .put("onesignalId", UUID.randomUUID().toString())
+                .put("id", UUID.randomUUID().toString())
+
+        // Add the Operations to the cache
+        jsonArray.put(validOperation.toJSON())
+        jsonArray.put(validOperationMissingOnesignalId.toJSON())
+        jsonArray.put(invalidOperationMissingOnesignalId.toJSON())
+        jsonArray.put(invalidOperationMissingName)
+        prefs.saveString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + "operations", jsonArray.toString())
+
+        // When
+        operationModelStore.loadOperations()
+
+        // Then
+        operationModelStore.list().count() shouldBe 2
+        operationModelStore.get(validOperation.id) shouldNotBe null
+        operationModelStore.get(validOperationMissingOnesignalId.id) shouldNotBe null
+        operationModelStore.get(invalidOperationMissingOnesignalId.id) shouldBe null
+        operationModelStore.get(invalidOperationMissingName["id"] as String) shouldBe null
+    }
+})