[Fix] Model store saving to and loading from prefs, and remove duplicates

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt

@@ -5,6 +5,7 @@ import com.onesignal.common.events.IEventNotifier
 import com.onesignal.core.internal.preferences.IPreferencesService
 import com.onesignal.core.internal.preferences.PreferenceOneSignalKeys
 import com.onesignal.core.internal.preferences.PreferenceStores
+import com.onesignal.debug.internal.logging.Logging
 import org.json.JSONArray
 
 /**
@@ -35,6 +36,7 @@ abstract class ModelStore<TModel>(
     IModelChangedHandler where TModel : Model {
     private val changeSubscription: EventProducer<IModelStoreChangeHandler<TModel>> = EventProducer()
     private val models: MutableList<TModel> = mutableListOf()
+    private var hasLoadedFromCache = false
 
     override fun add(
         model: TModel,
@@ -156,6 +158,10 @@ abstract class ModelStore<TModel>(
         changeSubscription.fire { it.onModelRemoved(model, tag) }
     }
 
+    /**
+     * When models are loaded from the cache, they are added to the front of existing models.
+     * This is primarily to address operations which can enqueue before this method is called.
+     */
     protected fun load() {
         if (name == null || _prefs == null) {
             return
@@ -164,17 +170,42 @@ abstract class ModelStore<TModel>(
         val str = _prefs.getString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, "[]")
         val jsonArray = JSONArray(str)
         synchronized(models) {
-            for (index in 0 until jsonArray.length()) {
+            val shouldRePersist = models.isNotEmpty()
+            for (index in jsonArray.length() - 1 downTo 0) {
                 val newModel = create(jsonArray.getJSONObject(index)) ?: continue
-                models.add(newModel)
+
+                /*
+                 * NOTE: Migration fix for bug introduced in 5.1.12
+                 * The following check is intended for the operation model store.
+                 * When the call to this method moved out of the operation model store's initializer,
+                 * duplicate operations could be cached.
+                 * See https://github.com/OneSignal/OneSignal-Android-SDK/pull/2099
+                 */
+                val hasExisting = models.any { it.id == newModel.id }
+                if (hasExisting) {
+                    Logging.debug("ModelStore<$name>: load - operation.id: ${newModel.id} already exists in the store.")
+                    continue
+                }
+
+                models.add(0, newModel)
                 // listen for changes to this model
                 newModel.subscribe(this)
             }
+            hasLoadedFromCache = true
+            // optimization only: to avoid unnecessary writes
+            if (shouldRePersist) {
+                persist()
+            }
         }
     }
 
+    /**
+     * Any models added or changed before load is called are not persisted, to avoid overwriting the cache.
+     * The time between any changes and loading from cache should be minuscule so lack of persistence is safe.
+     * This is primarily to address operations which can enqueue before load() is called.
+     */
     fun persist() {
-        if (name == null || _prefs == null) {
+        if (name == null || _prefs == null || !hasLoadedFromCache) {
             return
         }
 

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

@@ -4,12 +4,19 @@ import com.onesignal.common.events.EventProducer
 import com.onesignal.common.modeling.IModelChangedHandler
 import com.onesignal.common.modeling.IModelStoreChangeHandler
 import com.onesignal.common.modeling.ModelChangedArgs
+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.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.subscriptions.SubscriptionModel
 import com.onesignal.user.internal.subscriptions.SubscriptionModelStore
 import io.kotest.core.spec.style.FunSpec
 import io.kotest.matchers.shouldBe
+import org.json.JSONArray
+import java.util.UUID
 
 class ModelingTests : FunSpec({
 
@@ -136,4 +143,31 @@ class ModelingTests : FunSpec({
         // ensure no concurrent modification exception is thrown and "subcribers" is clear
         model.hasSubscribers shouldBe false
     }
+
+    test("ensure Model Store load pulls cached operations and doesn't duplicate models") {
+        // Given
+        val prefs = MockPreferencesService()
+        val operationModelStore = OperationModelStore(prefs)
+        val jsonArray = JSONArray()
+
+        val cachedOperation = SetTagOperation()
+        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()
+        newOperation.id = UUID.randomUUID().toString()
+        operationModelStore.add(newOperation)
+        operationModelStore.loadOperations()
+
+        // Then
+        operationModelStore.list().count() shouldBe 2
+        // The cached operation will not be the same instance
+        operationModelStore.get(cachedOperation.id)!!.name shouldBe cachedOperation.name
+        // The new operation added directly to the store should be the same instance
+        operationModelStore.get(newOperation.id) shouldBe newOperation
+    }
 })