diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt index b10b3df214..f7c41a410b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt +++ b/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( IModelChangedHandler where TModel : Model { private val changeSubscription: EventProducer> = EventProducer() private val models: MutableList = mutableListOf() + private var hasLoadedFromCache = false override fun add( model: TModel, @@ -156,6 +158,10 @@ abstract class ModelStore( 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( 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 } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ModelingTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ModelingTests.kt index 3883fdc872..46ffc10452 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ModelingTests.kt +++ b/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 + } })